-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update CAMARA_common.yaml: add Device headers, fix address #120
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
But as this will impact many existing APIs (as IP addresses are often used) we need to ask other companies for their opinions.
Thank you @patrice-conil for pointing this out. As this small change: Note: for IPv6 the |
Edits to be consistent with the current indentation
@@ -10,6 +10,31 @@ info: | |||
version: 0.4.0 | |||
paths: {} | |||
components: | |||
parameters: | |||
IPv4-Address: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using same format for all schemas? If we use UpperCamelCase for schema names, it is more coherent to use them as well here, e.g. Ipv4Address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By convention HTTP headers are Kebab-Case: RFC 2616, section 5.3. So IPv4-Address
uses the same naming style because it is an HTTP Header. "DeviceIpv4Address" is the schema that the parameter is based on (which follows CAMARA UpperCamelCase convention for schema names).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kevin,
Understood and OK to use kebab-case for the parameter property value. I checked with Jose, he was refering only to use UpperCamelCase for the schema name, just for aligment. Usually code generators also works better with schemas named following Camelcase. So it would read (just removing "-", for the new schemas included):
IPv4Address:
in: header
name: IPv4-Address
required: false
schema:
$ref: "#/components/schemas/DeviceIpv4Address"
WDYT?
The change affects code generation as those names are schema names that may be mapped to classes. The API itself does not change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Adopt UpperCamelCase for parameters schemas
The schemas for the new parameters do have UpperCamelCase names. But the parameter names (that reference the schemas) have Kebab-Case names, because they are specifically HTTP header parameters, which follow the convention in RFC 2616, section 5.3. |
Another comment: As it has been defining models por "headers" params, this model is not wide enough to cover with serialziation topics: https://swagger.io/docs/specification/describing-parameters/#header-parameters Header ParametersHeader parameters always use the * Default serialization method Header Parameters Header parameters always use the simple style, that is, comma-separated values. This corresponds to the {param_name} URI template. An optional explode keyword controls the object serialization. Given the request header named X-MyHeader, the header value is serialized as follows:style explode URI template Primitive value X-MyHeader = 5 Array X-MyHeader = [3, 4, 5] Object X-MyHeader = {"role": "admin", "firstName": "Alex"}
|
To summarize the comments: - Comment 1: It can be used "UpperCamelCase" as model for schema convention which does not preclude to name a parameter which is a header as kebab-case notation
- Comment 2: With regards to the schema model to refer to a "header".
cc @Kevsy (we talked in 25th March Commonalities meeting about this PR) |
@PedroDiez , @rartych , please can you propose any further changes as a new commit of the PR? I believe Comment 1 above is already implemented in the current commit, but I'm not sure what is the decision on Comment 2. |
@PedroDiez @Kevsy I see headers for device identification are used in https://github.com/camaraproject/SimpleEdgeDiscovery/pull/4/files The
Do you think this approach can be used here? But replacing |
Fixes #118 #119
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Addr
toAddress
in object names.Which issue(s) this PR fixes:
Fixes #118
Fixes #119
Special notes for reviewers:
Changelog input
Additional documentation
This section can be blank.