WebRTC Release r2.1 candidate for Fall25#91
WebRTC Release r2.1 candidate for Fall25#91stroncoso-quobis merged 20 commits intocamaraproject:mainfrom
Conversation
hdamker
left a comment
There was a problem hiding this comment.
@stroncoso-quobis Sorry, I started to comment and then realized that the PR is currently in draft mode. But still the comment that this will r2.1, not r1.3.
CHANGELOG.md
Outdated
| * webrtc-registration v0.2.0 | ||
| * webrtc-call-handling v0.2.0 | ||
| * webrtc-events v0.1.0 |
There was a problem hiding this comment.
Please update here the versions
There was a problem hiding this comment.
API versions updated
There was a problem hiding this comment.
Also stated that this is a pre-release
487452a to
91ccfb7
Compare
stroncoso-quobis
left a comment
There was a problem hiding this comment.
Suggestions noted and reviewed:
- Included
-rc.1suffix on API versions - Renamed release to 2.x as it is a new releasing cycle
- Identified r2.1 as a pre-release, and separated from public release 1.3
Open to review.
Commonalities changes still pending to be approved and merged at #88
Best regards,
CHANGELOG.md
Outdated
| * webrtc-registration v0.2.0 | ||
| * webrtc-call-handling v0.2.0 | ||
| * webrtc-events v0.1.0 |
There was a problem hiding this comment.
API versions updated
CHANGELOG.md
Outdated
| * webrtc-registration v0.2.0 | ||
| * webrtc-call-handling v0.2.0 | ||
| * webrtc-events v0.1.0 |
There was a problem hiding this comment.
Also stated that this is a pre-release
- Updates the webrtc-call-handling version to 0.3.0-rc.1 - Updates the webrtc-call-regsitration version to 0.3.0-rc.1 - Updates the webrtc-events-subscription version to 0.2.0-rc.1 - Reflects the API version updates in API_DOC eAPI readiness checklists
694dca3
|
Hi,
Best regards, |
hdamker
left a comment
There was a problem hiding this comment.
Please find my review comments below. Most of the points can easily be addressed and corrected.
The name conflict of webrtc-events (server url) versus webrtc-events-subscription (current file name and title) vs webrtc-events-subscriptions (as requested by guideline) needs a decision from the team how to handle it short- and mid-term.
documentation/API_documentation/webrtc-call-handling-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
documentation/API_documentation/webrtc-events-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
documentation/API_documentation/webrtc-registration-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Issues in test files of webrtc-registration
In all test files
- All test files reference v0.2.0 in feature line -> should match API version 0.3.0-rc.1
File Naming Inconsistency
- Files named: *-createSession.feature, *-updateSession.feature, *-deleteSession.feature
- API operations: createRegistration, updateRegistrationById, deleteRegistrationById
- Test files should match operation names
Scenario Path Inconsistencies
- deleteSession.feature: References /registrations/existing-session-id
- Should be: /sessions/existing-session-id
Schema Reference Errors
- createSession.feature:
/components/schemas/regSessionRequest - Should be:
#/components/schemas/regSessionRequest
Test Data Format
- createSession test uses deviceId: "1qazxsw23edc"
- API description states deviceId should be in UUID format
Duplicate Scenario ID
- deleteSession.feature has @webrtc_registration_getSession_400.2_empty_request
- Appears to be copy-paste error (should be deleteSession)
There was a problem hiding this comment.
-
All test files reference v0.2.0 in feature line -> should match API version 0.3.0-rc.1
- resource url fixed to v0.3rc1
-
File Naming Inconsistency
- Updated filenames and scenarios as the operationId using
registrationis more accurate with the API meaning and behavior
- Updated filenames and scenarios as the operationId using
-
Scenario Path Inconsistencies
- Fixed
There was a problem hiding this comment.
Test Data Format
- createSession test uses deviceId: "1qazxsw23edc"
- API description states deviceId should be in UUID format
Duplicate Scenario ID
- deleteSession.feature has @webrtc_registration_getSession_400.2_empty_request
- Appears to be copy-paste error (should be deleteSession)
@stroncoso-quobis seems that these two issues are not fixed, right?
That has to happen also in the test definition files and, very important, in all server URLs. |
There was a problem hiding this comment.
Comments on the CHANGELOG.md ... would be in general good to add the links to the PRs, so people can have a look into the details with one click.
BTW: this is the link of PRs as created when you do a "draft release" in GitHub (I removed the administrative ones):
- Support registration expiry updates and termination event notifications by @deepakjaiswal1 in #83
- fix: update webrtc-events-susbcription links by @stroncoso-quobis in #89
- 88 commonalities 060 by @stroncoso-quobis in #90
- Updated UML diagrams to latest public release by @stroncoso-quobis in #73
CHANGELOG.md
Outdated
| - OpenAPI [YAML spec file](https://github.com/camaraproject/WebRTC/blob/r2.1/code/API_definitions/webrtc-call-handling.yaml) | ||
|
|
||
| ### Changes | ||
| * Commonalities aligment 0.6 at PR #XX |
CHANGELOG.md
Outdated
|
|
||
| ### Changes | ||
| * Commonalities aligment 0.6 at PR #XX | ||
| * Extended `TerminationReason` enumeration to include `REGISTRATION_EXPIRED`, allowing precise classification of expiry-based terminations. |
|
Two questions regarding /code/API_definitions/README.MD:
|
@stroncoso-quobis I have provided in the multiple comments above the issues I've found during my detailed review of the completed release PR. It would be good to address them timely (this week). If it is of help to discuss some of the points, feel free to propose some slots to me in mail or slack. |
|
@camaraproject/web-rtc_maintainers @camaraproject/web-rtc_codeowner Can anyone pick up the necessary todos for this PR? The goal to get this done at latest beginning of next week (July 27th). Afterwards most of Release Management team will be on vacation and we have to postpone WebRTC to Spring26. |
There was a problem hiding this comment.
@hdamker Fixed all comments, mostly simply path updates.
Really sorry with the webrtc-events API file, test and naming convention, improving on each release. Apologies in advance.
Commiting changes right away.
CHANGELOG.md
Outdated
| - OpenAPI [YAML spec file](https://github.com/camaraproject/WebRTC/blob/r2.1/code/API_definitions/webrtc-call-handling.yaml) | ||
|
|
||
| ### Changes | ||
| * Commonalities aligment 0.6 at PR #XX |
CHANGELOG.md
Outdated
|
|
||
| ### Changes | ||
| * Commonalities aligment 0.6 at PR #XX | ||
| * Extended `TerminationReason` enumeration to include `REGISTRATION_EXPIRED`, allowing precise classification of expiry-based terminations. |
There was a problem hiding this comment.
-
All test files reference v0.2.0 in feature line -> should match API version 0.3.0-rc.1
- resource url fixed to v0.3rc1
-
File Naming Inconsistency
- Updated filenames and scenarios as the operationId using
registrationis more accurate with the API meaning and behavior
- Updated filenames and scenarios as the operationId using
-
Scenario Path Inconsistencies
- Fixed
documentation/API_documentation/webrtc-call-handling-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
documentation/API_documentation/webrtc-events-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
documentation/API_documentation/webrtc-registration-API-Readiness-Checklist.md
Outdated
Show resolved
Hide resolved
- Updates servers urls to correct ones(v0.3rc1, v0.2rc1) - Adds `Generic400` and review for all apis - Align x-correlator format across APIs - Limit deviceId format - Renamed registration api testing files - Renamed webrtc-events api file for consistency - Removed sessionNotification schema, not used - Align regsitration testing with operationId and path - Reviewed API documentation release candidate identificator
|
Hi @hdamker , Firstly, thanks for your infinite patience with our repo. We will de better on next releases.
Done. We approved and merge bug fixing #93 and #94 , with that, I rebase the release branch, so main changes are already included as part of rc.1.
Fixed:
Finally updated API documentation readiness files and CHANGELOG updated with bugfixing added ( #93 and #94 ) at 1dce688 Let me know if we need to address extra changes. Best regards, |
hdamker
left a comment
There was a problem hiding this comment.
Looks finally good to me from release management perspective.
Next actions to be completed for M3:
- PR merged (by API repository codeowner)
- Release created within GitHub (by API repository codeowner)
- Release Tracker updated (with API version, creation date of the release and the release tag link)
The merge-base changed after approval.
@stroncoso-quobis There is a lot to be improved in the release process as well, lot of manual work and checks which should be automated. If you have some time have a look on camaraproject/ReleaseManagement#275 and let me know what do you think - overdue or overkill? |
|
Hi @hdamker ,
I will definitely will take a look to the release process and let me know your thought. It is indeed a little too much, but also, it is not easy to keep all the 81 repositories in good shape. My admiration for your work.
Ok, so, as your approval was done before the rebase, a new approval is required. I will contact with the other codeowners and we will create tag and GitHub release. Thanks for your review! |
Now I'm little bit confused: I did my review around 18:00 UTC yesterday and your latest commits on your branch are from around 11:30 UTC. I can't see any changes or merge commits here in the history, except that you have dismissed my approval manually. But maybe I missed something. Happy to approve again after I have understood what changed. |
hdamker
left a comment
There was a problem hiding this comment.
Ready to approve again ... but just seen that the CHANGELOG can be improved regarding the links. "#XX" format will be rendered in GitHub descriptions and comments, but as far as I know not in markdown documents.
CHANGELOG.md
Outdated
| * Provided a concrete example for the `registration-ends` event subscription. | ||
|
|
||
| ### Changes | ||
| * fix: Updated deviceId format, examples and tests at #91 |
There was a problem hiding this comment.
Seems that the links are not working in the resulting changelog in this format, propose to change them to the format which GitHub is creating within draft releases, then they will work also in local clones/downloads:
| * fix: Updated deviceId format, examples and tests at #91 | |
| * fix: Updated deviceId format, examples and tests at https://github.com/camaraproject/WebRTC/pull/91 |
Please replace the other occurrences accordingly
There was a problem hiding this comment.
Good catch
I applied a regex replacement: It keeps it tidy for Markdown but properly linked
#(\d\d)
[#$1](https://github.com/camaraproject/WebRTC/pull/$1)
This replaced all previous not linked PR references. And CHANGELOG looks nice now.
|
Hi @hdamker ,
No clue at all... I was trying to investigate, but I can't reach any conclusion. It was 8PM on my timezone, it must be any kind of automation, no local traces at all on my side. Following that path, I look for the audit logs at GitHub, but I can not access to the admin section of the GitHub repo, so I can't figure exactly what happen and from where it happens. If you take a look there, maybe you can find something... Also I have 2FA activated and personal security audit logs does not show any activity... 🤔 intriguing...
Fixed CHANGELOG at 8aad9d7 Re-reviewed markdown rendering and look for any. If it looks ok to you, please approve, and we will release. Best regards, |
The merge-base changed after approval.
The merge-base changed after approval.
The merge-base changed after approval.
hdamker
left a comment
There was a problem hiding this comment.
Next try to approve from Release Management (after removing branch protection on release)
The merge-base changed after approval.
|
I will try to close and reopen the PR to potentially reset the review state. |
|
@stroncoso-quobis seems to work. @deepakjaiswal1 please approve as well again so that we can merge here. |
|
the branch protection on Answer from Perplexity which I used to fix (see temporary workaround). I kept the "Dismiss stale pull request approvals when new commits are pushed" rule on main active. Forced pushes are maybe not a good idea into a PR branch.
|
What type of PR is this?
Add one of the following kinds:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #82
Special notes for reviewers:
This PR includes all changes from main.
Changelog input
Additional documentation
This section can be blank.