Skip to content
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

Changes requested by Release Management for M4 #120

Open
tanjadegroot opened this issue Feb 18, 2025 · 10 comments
Open

Changes requested by Release Management for M4 #120

tanjadegroot opened this issue Feb 18, 2025 · 10 comments

Comments

@tanjadegroot
Copy link
Contributor

tanjadegroot commented Feb 18, 2025

Problem description
The M3 review was not fully completed before the release was created.
Review comments are listed below and are to be applied for M4 pubic release.
It was agreed in the RM meeting on 18/ Feb 2025 to handle the final review comments through a separate issue (this one).

Requested changes
They will be added as comments to this issue
Please address them though one or more dedicated PR(s) as you see fit.

To codeowners

The RM review is now ongoing - I will comment on this issue when the review is done.
Then you can finish the PR addressing the requested changes and when done request review by adding "release-management_maintainers" as a reviewer

@tanjadegroot
Copy link
Contributor Author

tanjadegroot commented Feb 18, 2025

API-Readiness-Checklist files
the file names should start with the api-name (in lower-kebab-case in the prefix)

The links in all the Checklist files should be relative to the release and should look like e.g.:
https://github.com/camaraproject/ConnectivityInsight/blob/r2.1/code/Test_definitions/xxx.feature

@tanjadegroot
Copy link
Contributor Author

tanjadegroot commented Feb 18, 2025

connectivity-insights.yaml (and any other file):
replace any link containing "main" by a link local to the release (e.g. for the picture reference).

connectivity-insights-subscriptions.yaml
replace any link containing "main" by a link local to the release (e.g. for the picture reference) or the meta-release for links to Commonalities or ICM assets.

application-profiles.yaml
The section "Identifying the device from the access token" in the info.description field refers to error 422 but this error is not part of the responses sections.
Either the section needs to be adapted to this API or the error needs to be added (fully or partially) to the API.
See https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml

@tanjadegroot
Copy link
Contributor Author

application-profiles.feature
in scenario line 18, should the Given not include the specific Id ?
in scenario line 25, should the Given not include the specific Id ? Also line 28 should say "Update"
there is no GET operation on the /application-profiles end-point. Should there be ? and if so there should also be a test
in scenario line 32, should the Given not include the specific Id ? Also, line 35 should say "Delete"

connectivity-insights-subscriptions.feature
in scenario line 18: should this test not return a list of (0 or 1, or more) Subscription objects ?
in scenario line 25, should the Given not include the specific Id ?
in scenario line 32, should the Given not include the specific Id ?

all 3 .feature files:
I think the "{path_resource}" part needs to be replaced by the API specific path-resources, minimally in the Baground section.
I am not a feature file specialists so maybe aske the Commonalities team.

For the M4 updates, as an example you can look at the files here https://github.com/camaraproject/DeviceStatus/tree/main/code/Test_definitions.
The API end-points and subscription tests can be more elaborate.

@tanjadegroot
Copy link
Contributor Author

README.md
lines 10 and 16: change "API family" to "APIs". the concept of API family does no longer exist.
line 14 change "customer" to "API consumer"
lines 15 and 16 update the description to better fit with the the latest version of the APIs, remove policy, etc.

@tanjadegroot
Copy link
Contributor Author

For M4: Use case file: rework the file to be in synch with the latest version of the API, e.g.

  • change "metrics" to "profile" or at least use "profile"
  • remove "policy"
  • change "CSP's API platform" to "API Exposure Platfrom" (can be from CSP/Aggregator/Marketplace, etc). The latter may all be Actors.
  • "monitoring" concept ?
  • "App-Flow" ?
  • etc.

the Use case file(s) shall cover all APIs.

@tanjadegroot
Copy link
Contributor Author

tanjadegroot commented Feb 19, 2025

Hi
I finished the (M3) release review.

Please be aware that there will be mandatory updates for M4 on any subscription API due to some updates included in the public release of Commonalities.

Please add "release-management_maintainers" as a reviewer once you have handled the above comments.

@maheshc01
Copy link
Collaborator

API-Readiness-Checklist files the file names should start with the api-name (in lower-kebab-case in the prefix)

The links in all the Checklist files should be relative to the release and should look like e.g.: https://github.com/camaraproject/ConnectivityInsight/blob/r2.1/code/Test_definitions/xxx.feature

the latest commit will address the API readiness checklist file name change.
With respect to the referenced files in the checklist, they are relative to the codebase and will point to the files in the respective release. so dont understand the ask for this change. current relative paths are consistent with what other APIs are using.

@maheshc01
Copy link
Collaborator

connectivity-insights.yaml (and any other file): replace any link containing "main" by a link local to the release (e.g. for the picture reference).

connectivity-insights-subscriptions.yaml replace any link containing "main" by a link local to the release (e.g. for the picture reference) or the meta-release for links to Commonalities or ICM assets.

application-profiles.yaml The section "Identifying the device from the access token" in the info.description field refers to error 422 but this error is not part of the responses sections. Either the section needs to be adapted to this API or the error needs to be added (fully or partially) to the API. See https://github.com/camaraproject/Commonalities/blob/main/artifacts/CAMARA_common.yaml

the urls to the images in connectivity-insights.yaml and connectivity-insights-subscriptions.yaml has been updates to use: https://raw.githubusercontent.com/camaraproject/ConnectivityInsights/r2.2/documentation/API_documentation/ConnectivityInsights-SequenceDiagram.png

connectivity-insights-subscriptions.yaml has reference to a link in main branch of commonalities at 2 places as shown below:
SubscriptionId:
type: string
description: |
The unique identifier of the subscription in the scope of the
subscription manager. When this information is contained within
an event notification, this concept SHALL be referred as
subscriptionId as per
Commonalities Event Notification Model.
example: qs15-h556-rt89-1298
this was part of the template that was asked to be used. checked that other subscriptions apis in camara and they have the same in the description. letting you know that other APIs will also require this change.
In my opinion rather than update the url to not use main we should remove the reference to the url to have a more generic description as below:
SubscriptionId:
type: string
description: |
The unique identifier of the subscription in the scope of the
subscription manager. When this information is contained within
an event notification, this concept SHALL be referred as
subscriptionId as per the guidelines in commonalities.
example: qs15-h556-rt89-1298

Please confirm if this update makes sense or suggest alternate update so that i can plan accordingly. @tanjadegroot @hdamker

With respect to application-profiles.yaml having references to 422 error in the description, i have kept this content as a generic description to keep it consistent with other Camara APIs even though there is no concept of device specifically in application-profiles api. there is no scenario for adding this error response for any of the end points in this API.
I can go ahead and remove the entire section of "Identifying the device from the access token" for this API if thats ok.
Please let me know your inputs @tanjadegroot @hdamker

@maheshc01
Copy link
Collaborator

  • API Exposure Platfrom

the single user story was specific to the initial API proposal as made to API backlog group. As part if the API working group it evolved to separating out application profiles and connectivity insights.
Connectivity insights was further split to synchronous API and connectivity-insights-subscriptions based on Camara standard guidelines. As this was not done as any part of any planned scope expansion the user stories were limited to one file and overtime become obsolete.
I have create 2 user story files now:

  1. Application Profiles
  2. Connectivity Insights (combines synchronous as well as subscriptions/notifications)

@maheshc01
Copy link
Collaborator

Hi I finished the (M3) release review.

Please be aware that there will be mandatory updates for M4 on any subscription API due to some updates included in the public release of Commonalities.

Please add "release-management_maintainers" as a reviewer once you have handled the above comments.

have made a number updates as per the review comments and in case no update was made i have left a comment giving a justification or seeking a clarification.
Also as part of the latest commonalities changing the url to uri was already done in subscriptions api as part of M3.

Please review the latest commit and my responses and let me know your feedback on next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants