-
Notifications
You must be signed in to change notification settings - Fork 43
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
Simple EDS API Aligned with commonalities #127
Conversation
* Unused parameters removed * Unused resource types removed * `filter` parameter added (to future-proof `GET /mecplatforms` with no filter as a request to read the collection resource * URI changed to kebab-case * OperationId updated to Camel Case * Identifiers moved to header for security reasons (per Device Identifier API) * Basepath updated including major version * Standard error codes added * Documentation simplified and examples added
"closest" | ||
] | ||
|
||
- name: IP-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.
Should this be a OneOf for IP-Address, Network-Access-Identifier, or Phone-Number.
Also I don't think capital kebab-case is a supporting naming convention. I believe that these parameters would be classified as objects and would be CabelCase. So IpAddress
Objects are defined in CamelCase inside properties field. For example: Greetings, ExampleObject
Per section https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#41-url-definition
Perhaps kebab-case would also be acceptable. for ip-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.
Thanks for reviewing @RandyLevensalor -
Should this be a OneOf for IP-Address, Network-Access-Identifier, or Phone-Number.
At least one is required, but two or all of them can be passed. This may help the operator in case they have trouble resolving the device. The CAMARA Device Identifier API takes the same approach.
Also I don't think capital kebab-case is a supporting naming convention. I believe that these parameters would be classified as objects and would be CabelCase. So
IpAddress
Good point and I just checked this with a colleague on the CAMARA Technical Steering Committee. The exception here is that these are HTTP Request headers, which, whilst not prescribed by RFC 7230, are de facto written in Captal-Kebab case (ref: IANA message headers). The CAMARA Commonalities API Design Guidelines go on to say that:
Business data sent as part of the HTTP protocol will be exempt from these rules. In these cases, compliance with the standard protocol will apply.
...hence, use of Capital-Kebab as that is the expected convention for headers. I'll make a PR on the Design Guidelines to make that clearer.
ResourcesMecPlatform: | ||
|
||
EdgeResource: | ||
description: | |
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.
Have you considered providing a URI and / or a unique ID for the EdgeResource?
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.
Business data sent as part of the HTTP protocol will be exempt from these rules. In these cases, compliance with the standard protocol will apply.
Not a URI - since the platform itself (unlike a resource running on it) is not linkable.
But that's a great point about unique ID, because the various edge cloud providers do not have an agreed naming convention (such as a URN) which would ensure a globally unique value. E.g. in the South of England you could get the following names returned depending on the platform provider:
eu-west-2-wl1-lon-wlz-1
Europe-west2
UK South
So I propose to add the platform provider name, e..g AWS, Azure, GCP, {operator name} to help clarify. This can be particularyly useful should an operator partner with more than one platform provider. WDYT? @kevin8xu I think we may have discussed this at one point...?
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.
Looks good to me.
1. made `MecPlatforms` an array of `MecPlatform`. Only one `MecPlatform` is returned for this Simple EDS API but this allows extensibility 2. Added the Provider as context to the Ern (name of closest MEC Platform) as there is no agreed globally unique identifier for a MEC Platform 3. Simplified the introduction.
Following inital reviews (thanks!) I've made thgree changes to the commit (same Pull Request):
|
Align with #127 and fix comment from initial review (404 instead of 500 for 'invalid device identifier')
Can we assume that there is an implicit assumption that the MEC platform names are to be unique within an telco environment? As the API client responsibility is assumed to manage the mapping a MEC platform name as returned in the API response to the application endpoints or IP address details so in my opinion it implicitly imposes a uniqueness constraints for the operator to manage the uniqueness of MEC platform names. If this is the case then it should be mentioned in the API description as the constraint then needs to be considered for SED API implementation. |
added missing quote mark in example success response
Added to latest document a 'note for API publishers' that the |
successResponse: | ||
type: object | ||
properties: | ||
MecPlatforms: | ||
$ref: '#/components/schemas/MecPlatforms' |
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.
Just a small comment: if you think that additional APIs may be added to this spec in the future, I would suggest that we remove this successResponse and encode the return type into the actual path+method definition. That is because another API will likely have a different successResponse object type. Unlike the errorReponse definition which is generic and can be referenced across all APIs, this successResponse is specific to a single API, so would be better defined just within path+method definition (i.e. mec-platforms/get), i.e. around line 167 would become:
responses:
"200":
description: A list of MEC Platforms, sorted by closest first
content:
application/json:
schema:
$ref: '#/components/schemas/MecPlatforms'
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.
Thanks @gainsley, that's a good suggestion: updated accordingly in latest commit
@crissancas @sergiofranciscoortiz please can you approve and merge? Thanks! |
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.
Approved after last changes taking into account previous comments.
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.
Reviewed
filter
parameter added (to future-proofGET /mecplatforms
with no filter as a request to read the collection resource)