feat(coserv): add coserv API discovery document#272
Conversation
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
LGTM, thanks!
I have left a couple of easy comments inline.
| if !strings.HasSuffix(queryResponse, "{query}") { | ||
| return ErrInvalidRequestResponseEndpoint | ||
| } |
There was a problem hiding this comment.
In theory, there’s another thing that must be checked:
"There MUST NOT be any other variables that require substitution.”
Additinaolly, there’s an implicit requirement that the string should parse as a URI and the URI must not be absolute.
| if len(o.ApiEndPointsMap) < 1 { | ||
| return ErrEmptyApiEndPoints | ||
| } | ||
| queryResponse, ok := o.ApiEndPointsMap["CoSERVRequestResponse"] |
There was a problem hiding this comment.
Unsure what queryResponse is supposed to mean?
Perhaps a slightly more evocative name could be requestResponseURI or similar.
@thomas-fossati comments are addressed. Please review. |
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments!
| ErrNoRequestResponseEndpoint = errors.New("could not find `CoSERVRequestResponse' endpoint") | ||
| ErrInvalidRequestResponseEndpoint = errors.New("invalid `CoSERVRequestResponse' endpoint") | ||
| ) | ||
|
|
There was a problem hiding this comment.
As a reference if you could please add a comment what this structure is used for
and reference any Discovery API document, it is linked to?
|
|
||
| type DiscoveryDocument struct { | ||
| Version string `cbor:"1,keyasint" json:"version"` | ||
| CapabilitiesList []Capability `cbor:"2,keyasint" json:"capabilities"` |
There was a problem hiding this comment.
The name on Line 29, can mirror the JSON key, ie, it can be
Capabilities instead of CapabilitiesList
There was a problem hiding this comment.
There are methods with those names, which return iterators to the entries. So the same name cannot be used for fields.
| type DiscoveryDocument struct { | ||
| Version string `cbor:"1,keyasint" json:"version"` | ||
| CapabilitiesList []Capability `cbor:"2,keyasint" json:"capabilities"` | ||
| ApiEndPointsMap map[string]string `cbor:"3,keyasint" json:"api-endpoints"` |
There was a problem hiding this comment.
same comment as 29
yogeshbdeshpande
left a comment
yogeshbdeshpande
left a comment
There was a problem hiding this comment.
Left few very minor comments, but in General Looks Good to Me, hence I have approved the PR.
Implementation of CoSERV well-known response as per https://datatracker.ietf.org/doc/draft-ietf-rats-coserv/06/. Also added test cases for discovery document from the draft. Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
golangci < 2.5.0 uses github.com/tdakkota/asciicheck as a dependency. But this was moved to github.com/golangci/asciicheck. So if the former is not available in the cache or in the proxy, linting tests failed. golangci v2.5.0 requires go >= 1.24, hence bumped go version. Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
|
@thomas-fossati and @yogeshbdeshpande thanks for reviewing the PR. All review comments are addressed. Please check and merge if everything looks good. Thanks |
4 participants
Fixes #262