application end point registration API yaml #321
Conversation
maheshc01
requested review from
gunjald,
Kevsy,
crissancas,
FabrizioMoggio,
nicolacdnll,
gainsley and
JoseMConde
as code owners
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports MegaLinter is graciously provided by |
| url: https://www.apache.org/licenses/LICENSE-2.0.html | ||
| description: | | ||
| Application endpoint registration allows application developers to | ||
| register one or more Application Endpoints, and retrieve, update,and |
There was a problem hiding this comment.
Should we have a definition for "Application Endpoints" to make it more explicit?
There was a problem hiding this comment.
I also think that these Application Endpoints are not generic and they corresponds to the application instances running on edge cloud zones in probably distributed manner. So should we qualify the Application Endpoints in some way to indicate developers about the expectations from these endpoints i.e. edge instances?
| description: | | ||
| Application endpoint registration allows application developers to | ||
| register one or more Application Endpoints, and retrieve, update,and | ||
| delete those egistrations. |
There was a problem hiding this comment.
I suppose egistrations is should be "registrations"
|
|
||
| This information can we used for various usecases like optimal end | ||
| point discovery to help end users connect to the most most optimal | ||
| instance of the application. Addtionally the information can be used |
There was a problem hiding this comment.
I feel that if can refer or qualify applications as "edge applications" then it will make it more related to edge deployments while using generically may imply any deployment including clouds. Just a thought.
| delete those egistrations. | ||
|
|
||
| This information can we used for various usecases like optimal end | ||
| point discovery to help end users connect to the most most optimal |
There was a problem hiding this comment.
"most most optimal" should be changed to "most optimal"
| items: | ||
| $ref: "#/components/schemas/ApplicationInstance" | ||
| responses: | ||
| "200": |
There was a problem hiding this comment.
We should also consider the additional status codes which could be valid for the POST requests in alignment with other APIs and consistency.
| paths: | ||
| /application-endpoints: | ||
| post: | ||
| operationId: register-application-endpoints |
There was a problem hiding this comment.
For the POST method should we also include "x-correlator" header as done with other APIs.?
There was a problem hiding this comment.
Included "x-correlator" for all the APIs
| edgeCloudRegionId: | ||
| $ref: "#/components/schemas/EdgeCloudRegionName" | ||
| ResourcesapplicationEndpoint: | ||
| type: object |
There was a problem hiding this comment.
Should we assume that an application can expose only one endpoint or there could be more than one for different purposes? If there can be more than one which only application provider knows then this parameter should allow the multiplicity by having it as an array of objects. Also in that case with each endpoint there should be a discriminator field to allow client applications to know the purpose of a specific endpoint.
There was a problem hiding this comment.
Although the top level object is an array but then if application has multiple endpoints some of the information will be repeated that many times as they will be common to an application.
There was a problem hiding this comment.
I also had the same question about repetitive data in the array. For example, is it expected that applicationId is the same for each ApplicationInstance in the array? If any field must be the same for each item in the array, it should be factored out into a single field in a parent object that encapsulates the endpoints array. If every field in the array can be different, then what is the intent of assigning a single applicationEndpointsId to the entire array?
…ts-functionality-to-be-a-new-api EAR: Fixing Typo
|
@urvika-v tagging Urvika for review and updates |
| edgeCloudZoneId: | ||
| $ref: "#/components/schemas/EdgeCloudZoneId" | ||
| edgeCloudZoneName: | ||
| $ref: "#/components/schemas/EdgeCloudZoneName" | ||
| edgeCloudProvider: | ||
| $ref: "#/components/schemas/EdgeCloudProvider" | ||
| edgeCloudRegionId: | ||
| $ref: "#/components/schemas/EdgeCloudRegionName" |
There was a problem hiding this comment.
I have a question on the intent here. First, are these edgeCloudZone-related fields specified during the POST /application-endpoints API? If they are specified, is it expected that the Operator Platform handling the API call will verify that it has the specified Zone in it's list of zones (i.e., if I specify a random string for the Zone ID, it will reject it)? Or does it treat them as just a reference to some external Zone that it doesn't know about, so I could just put in anything there and I will just get it back out during the "find" API call?
| ApplicationInstance: | ||
| description: Application instance represented | ||
| by application Endpoint definition | ||
| type: object |
There was a problem hiding this comment.
I would suggest adding a required section to note which fields are required.
There was a problem hiding this comment.
Added "Required" section to the schema.
| applicationId: | ||
| type: string | ||
| description: Unique ID representing the Edge Application |
There was a problem hiding this comment.
Is this really the applicationId or is it the appInstanceId? In the EAM API, the appId refers to definition of an App, like a helm chart, while the appInstanceId refers to an actually deployed instance of the helm chart. I ask because this object is called ApplicationInstance but I don't see any appInstanceId field. I think it would confusing if applicationId in Discovery means something different from appId in EAM.
| description: | | ||
| region of the Edge Cloud Zone. | ||
| type: string | ||
| TypesApplicationEndpointsId: |
There was a problem hiding this comment.
I would suggest renaming TypesApplicationEndpointsId to just ApplicationEndpointsId, as the Types prefix seems redundant.
| format: uuid | ||
| readOnly: true | ||
| additionalProperties: false | ||
| TypesApplicationProfileId: |
There was a problem hiding this comment.
I would suggest renaming TypesApplicationProfileId to just ApplicationProfileId, as the Types prefix seems redundant.
There was a problem hiding this comment.
Modified "TypesApplicationProfileId" to "ApplicationProfileId"
| paths: | ||
| /application-endpoints: | ||
| post: | ||
| operationId: register-application-endpoints |
There was a problem hiding this comment.
I would suggest to specify operationIds in camelCase, rather than dash-separated, i.e. this should be registerApplicationEndpoints. Both the EAM API and the official doc examples specify operationIds in camelCase, and it's better to be consistent across the APIs.
| - name: applicationEndpointsId | ||
| in: path | ||
| required: true | ||
| description: applicationEndpointsId param //added desc |
There was a problem hiding this comment.
This description looks like it needs to be fixed, and the //added desc removed.
There's 6 instances //added desc in this file that need to be fixed.
| application. | ||
|
|
||
| paths: | ||
| /application-endpoints: |
There was a problem hiding this comment.
I would have expected there to also be a get method for /application-endpoints, to be able to list all of the registered applicationEndpointsId. Otherwise the client needs to maintain a database of the registered applicationEndpointsId to be able to update/delete them later.
There was a problem hiding this comment.
Agreed. Added GetAll api which lists all the registered applications.
| tags: | ||
| - Application Endpoint Registration | ||
| summary: Update a application Endpoint | ||
| description: Update registered application Endpoint information. |
There was a problem hiding this comment.
Please clarify if the update completely replaces the existing array or does something else like appending to it.
Also, a minor complaint, typically the summary and description would come at the top, right after the method type (i.e. the put or get). Having it all the way at the bottom after the responses means I'm trying to understand all the details of the method before I know the general intent of the method, which is hard. I would suggest moving them up.
There was a problem hiding this comment.
Moved the summary and the description to the top of the Schema.
| tags: | ||
| - name: Application Endpoint Registration | ||
| description: | | ||
| Operations to register, read and manage an deployed instances of the |
There was a problem hiding this comment.
read and manage an deployed instances -> either read and manage a deployed instance or read and manage deployed instances. Not sure if the intent is singular or plural but right now it's a mix.
There was a problem hiding this comment.
Modified to "read and manage deployed instances"
Updated Error codes,Description,Security Schema, required parameter and added GetAllApplicationEndpoints API
…ts-functionality-to-be-a-new-api AER: Updated Error Codes, Description and added GetAllRegisteredApplicationEndpoints API
…ts-functionality-to-be-a-new-api AER: Fixed Linting issues
What type of PR is this?
This PR is to review and update application endpoint registration API yaml.
Add one of the following kinds:
What this PR does / why we need it:
Initial commit of the yaml to provide ability to register application endpoints.
Which issue(s) this PR fixes:
issue #286
Fixes #
Special notes for reviewers:
This is only the initial commit and additional updates are required related to documentation, error codes etc to ensure it meets the CAMARA guidelines.
Changelog input
Additional documentation
This section can be blank.