-
Notifications
You must be signed in to change notification settings - Fork 1
Release r1.2 (Fall'25 M4) #34
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
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Minor fixes in descriptions Co-authored-by: Eric Murray <eric.murray@vodafone.com>
|
Thanks for your review, @eric-murray! Very good points. I had missed those points, all of which have been resolved and applied to the PR. |
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Co-authored-by: Herbert Damker <herbert.damker@telekom.de>
bigludo7
left a comment
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 this PR - reviewed on the behalf of the release management team
- (minor): I've added a NEW in the README.md as for the other APIs
- (major): Subscription APIs must include 'notificationsBearerAuth' security scheme so probably it should be added in the yaml.
Thanks
|
One additional request: please remove for the |
|
Hi @bigludo7, The other API Owner is on vacation, and we need approval to merge this PR. |
|
@albertoramosmonagas @bigludo7 It seems that the API is using a simple one-time callback mechanism for the response and not subscription to events. With that it is not following the CAMARA Event Subscription & Notification Guide in multiple points. I'm not sure if this is by intention and even then I would say that the API should use proper CloudEvent structure and a defined event type for the callback. Or in other words:
With that I'm not comfortable to bypass the codeowner approvals and would also ask @bigludo7 to have a second look on the API definition. |
|
Agree with @hdamker findings and sorry to have missed. In particular the event structure must be aligned with our definition defined here and here. As an example we should have this: This modification should be probably done in a separate issue/PR. |
|
Thanks a lot for raising this point. From our side, we see it as follows:
To summarize: we implemented it this way originally because we thought it was the correct approach at that time. We are of course open to revisiting this after the current meta-release, and if the group agrees, we can apply changes in a future meta-release. |
We have also other APIs which are using (implicit subscribed) event notifications to deliver there responses. We agreed with CAMARA Commonalities on one event structure across all APIs to avoid that API Consumers who are using multiple CAMARA APIS need to implement endpoints for different delivery models to receive asynchronous responses and notifications. To avoid a misunderstanding here: CloudEvents is only the delivery method, it does not restrict in any form the data model used within the payload. A decision of a "Predictive & Population WG" to use a different delivery model is IMHO not compliant with the agreed Commonalities in CAMARA.
I agree that the change is a larger one and need to be discussed in the responsible team. But I want to be clear that the review of Release Management is not a design review but to ensure the needed release artifacts are there and consistent. Within the process the RM team has to rely on the statement of the API working group that the API definition and design is compliant with the need Commonalities release (here: r3.3, but the requirement to use CloudEvent existed already in r2.3). Currently it is by chance that certain non-compliant issues will be found, as in this case. My proposed forward would be to create an issue for the needed change to get compliant with Commonalities r3.3 and then to update the API Readiness Checklist with a comment on the line regarding compliance with Commonalities. It then up to the Release Management, potentially TSC to discuss the consequences.
Thanks for pointing to the fact that also Population Density Data has the same issue. That need to be corrected as well going forward to avoid fragmentation of the CAMARA APIs. That's sad as this API Repository is already in Incubating stage. As said, it would be beyond the capabilities of the small team to do a deep design review for each of the 60 APIs. But we might need to introduce such a design review at latest before the incubation of an API Repository.
I'm personally fine to include the initial version of PredictiveConnectivityData in the Fall25 meta-release even with the open issue (other initial v0.1.0 API version have for sure similar or other issues), but I wouldn't wait with the release of the next version until the next meta-release, but fix this issue as soon as possible and release a v0.2.0 (in accordance with the dual-phase model). |
|
Hello @albertoramosmonagas We have a consistency issue here in referring commonalities r3.3 but not following its definition. Perhaps we can finish this release as it, but then quickly introduce a patch release (and not waiting Spring26) to fix this? EDIT: I forget to publish my comment before lunch and published it back.... hopefully I'm pretty aligned with Herbert :) |
Yes, pretty aligned. Only point is that I think that this change justifies a 0.2.0 version, as it won't be backward compatible with the current definition. |
|
Thanks a lot for the detailed feedback and clarifications.
In the meantime, our understanding is that we proceed with this PR and include the initial version in the Fall’25 meta-release. We can start moving these tasks in both repos right after Fall’25. Would that approach work for you, or is there anything else you would expect us to capture or follow up on? |
|
One thing @albertoramosmonagas ... as suggested by @hdamker you can update the API Readiness Checklist with a comment on the line regarding compliance with Commonalities. @hdamker Once we have the comment, do I approve or do we have to wait TSC? |
|
Comment created in the API Readiness Checklist and issue created for Predictive Connectivity Data (#38) and for Population Density Data (camaraproject/PopulationDensityData#105) |
|
@albertoramosmonagas Perhaps you can improve the formatting as it adds 2 lines - probably not what you're expecting. You can add an asterisk & put a note after the table. |
I think it's done on my side |
|
Look good for me ! |
@albertoramosmonagas @bigludo7 Looks good for me too, a good and pragmatic solution. From my perspective we can go forward. |
Technically you need two approvals: one from release management reviewer (that will come from @bigludo7 who runs the review) and one from a code owner. If @eric-murray is not available, I can help to bypass the rule later. |
I understand @eric-murray is on holidays for most of the month and may only be checking in occasionally to support some of the releases he owns. From what we discussed, it sounded like he had already given his approval prior to the recent format updates, so everything seemed aligned on his side. From our end, we truly appreciate your support and don’t foresee any issues in moving forward with your and @bigludo7 approval. |
bigludo7
left a comment
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 @albertoramosmonagas
Approved on behalf of Release Management 👏
Next steps for the team:
• [ ] PR merged (by API repository codeowner)
• [ ] Release created within GitHub (by API repository codeowner)
• [ ] Release Tracker updated (with creation date of the release and the release tag link)
|
I thought that with your approvals I would be able to merge, but I'm still stuck with this: Merging is blocked due to falling in merge requirements (Awaiting code owner review by eric-murray). |
@albertoramosmonagas |
|
@bigludo7
|
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Publication of Fall'25 M4 public release of predictive-connectivity-data v0.1.0
Which issue(s) this PR fixes:
Fixes #9
Special notes for reviewers:
None
Changelog input
Additional documentation
None