Skip to content

WebRTC Release r2.1 candidate for Fall25#91

Merged
stroncoso-quobis merged 20 commits intocamaraproject:mainfrom
stroncoso-quobis:82-release-candidate-fall25
Jul 30, 2025
Merged

WebRTC Release r2.1 candidate for Fall25#91
stroncoso-quobis merged 20 commits intocamaraproject:mainfrom
stroncoso-quobis:82-release-candidate-fall25

Conversation

@stroncoso-quobis
Copy link
Collaborator

@stroncoso-quobis stroncoso-quobis commented Jul 3, 2025

What type of PR is this?

Add one of the following kinds:

  • subproject management

What this PR does / why we need it:

  • Fall 2025 meta-release candidate

Which issue(s) this PR fixes:

Fixes #82

Special notes for reviewers:

This PR includes all changes from main.

Changelog input

 release-note
Updated README with latest links
Updated CHANGELOG with release upgrades
- webrtc-registration
fix: Updated deviceId format, examples and tests
- webrtc-call-handling
fix: Added 400 error response
- webrtc-events
fix: Updated deviceId format, examples and tests

Additional documentation

This section can be blank.

docs
Updated API-Readiness with Wiki link and ICM & COMMS new release

@stroncoso-quobis stroncoso-quobis requested a review from a team July 3, 2025 13:00
@stroncoso-quobis stroncoso-quobis added the subproject management Indicating issues with subproject repository or release management process label Jul 3, 2025
@stroncoso-quobis stroncoso-quobis changed the title Initial draft for PRs #83 #85 #86 Release r1.3 candidate for Fall25 Jul 3, 2025
@stroncoso-quobis stroncoso-quobis changed the title Release r1.3 candidate for Fall25 WebRTC Release r1.3 candidate for Fall25 Jul 3, 2025
Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Comment on lines +27 to +29
* webrtc-registration v0.2.0
* webrtc-call-handling v0.2.0
* webrtc-events v0.1.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update here the versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API versions updated

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also stated that this is a pre-release

@stroncoso-quobis stroncoso-quobis force-pushed the 82-release-candidate-fall25 branch from 487452a to 91ccfb7 Compare July 15, 2025 10:46
@stroncoso-quobis stroncoso-quobis changed the title WebRTC Release r1.3 candidate for Fall25 WebRTC Release r2.1 candidate for Fall25 Jul 15, 2025
Copy link
Collaborator Author

@stroncoso-quobis stroncoso-quobis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions noted and reviewed:

  • Included -rc.1 suffix 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
Comment on lines +27 to +29
* webrtc-registration v0.2.0
* webrtc-call-handling v0.2.0
* webrtc-events v0.1.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API versions updated

CHANGELOG.md Outdated
Comment on lines +27 to +29
* webrtc-registration v0.2.0
* webrtc-call-handling v0.2.0
* webrtc-events v0.1.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also stated that this is a pre-release

deepakjaiswal1
deepakjaiswal1 previously approved these changes Jul 15, 2025
@stroncoso-quobis stroncoso-quobis marked this pull request as ready for review July 16, 2025 06:51
- 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
@stroncoso-quobis
Copy link
Collaborator Author

Hi,

  • Merged main sources to convert this PR into an all-in-one release candidate.
  • Version tags updated to align with releasing notes.

Best regards,

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 registration is more accurate with the API meaning and behavior
  • Scenario Path Inconsistencies

    • Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@hdamker
Copy link
Contributor

hdamker commented Jul 16, 2025

  • Version tags updated to align with releasing notes.

That has to happen also in the test definition files and, very important, in all server URLs.

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add PR number if possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@hdamker
Copy link
Contributor

hdamker commented Jul 16, 2025

Two questions regarding /code/API_definitions/README.MD:

  • is the page up-to-date? I can't find "Notification Channel" in the repository
  • wouldn't it be better to use relative links instead of absolute links into main? The benefit would be that people will stay within the context (fork, branch, release, ...)

@hdamker
Copy link
Contributor

hdamker commented Jul 21, 2025

Hi,

  • Merged main sources to convert this PR into an all-in-one release candidate.
  • Version tags updated to align with releasing notes.

Best regards,

@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.

@hdamker
Copy link
Contributor

hdamker commented Jul 24, 2025

@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.

Copy link
Collaborator Author

@stroncoso-quobis stroncoso-quobis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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 registration is more accurate with the API meaning and behavior
  • Scenario Path Inconsistencies

    • Fixed

- 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
@stroncoso-quobis stroncoso-quobis requested a review from hdamker July 24, 2025 15:49
@stroncoso-quobis
Copy link
Collaborator Author

Hi @hdamker ,

Firstly, thanks for your infinite patience with our repo. We will de better on next releases.
Let's review changes from latest review:

There are already too many changes here in the PR (which is actually only meant for the administrative changes for the release) and therefore almost impossible to review in details. It is therefore good that other changes happen in their own PRs and then the release PR is rebased with main.

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.

Test Data Format and Duplicate Scenario ID - seems that these two issues are not fixed, right?

Fixed:

  • Duplicate Scenario ID at 3f5b9fa
  • Test Data format (and format update at webrtc-events) at cc0b767

Please change InProgress before the release

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,

@stroncoso-quobis stroncoso-quobis requested a review from hdamker July 28, 2025 11:32
hdamker
hdamker previously approved these changes Jul 28, 2025
Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@stroncoso-quobis stroncoso-quobis dismissed hdamker’s stale review July 28, 2025 17:59

The merge-base changed after approval.

@hdamker
Copy link
Contributor

hdamker commented Jul 28, 2025

We will de better on next releases.

@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?

@stroncoso-quobis
Copy link
Collaborator Author

Hi @hdamker ,

There is a lot to be improved in the release process as well

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.

Looks finally good to me from release management perspective.

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.
cc.: @pradeepachar-mavenir , @deepakjaiswal1

Thanks for your review!
Best regards,

@hdamker
Copy link
Contributor

hdamker commented Jul 29, 2025

Ok, so, as your approval was done before the rebase, a new approval is required.

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.

Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
* 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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@stroncoso-quobis
Copy link
Collaborator Author

Hi @hdamker ,

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

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...

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.

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,

deepakjaiswal1
deepakjaiswal1 previously approved these changes Jul 29, 2025
@stroncoso-quobis stroncoso-quobis dismissed deepakjaiswal1’s stale review July 29, 2025 12:37

The merge-base changed after approval.

deepakjaiswal1
deepakjaiswal1 previously approved these changes Jul 29, 2025
@stroncoso-quobis stroncoso-quobis dismissed deepakjaiswal1’s stale review July 29, 2025 12:42

The merge-base changed after approval.

hdamker
hdamker previously approved these changes Jul 29, 2025
Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another try to approve

@stroncoso-quobis stroncoso-quobis dismissed hdamker’s stale review July 29, 2025 14:27

The merge-base changed after approval.

hdamker
hdamker previously approved these changes Jul 29, 2025
Copy link
Contributor

@hdamker hdamker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next try to approve from Release Management (after removing branch protection on release)

@stroncoso-quobis stroncoso-quobis dismissed hdamker’s stale review July 29, 2025 14:34

The merge-base changed after approval.

@hdamker
Copy link
Contributor

hdamker commented Jul 29, 2025

I will try to close and reopen the PR to potentially reset the review state.

@hdamker hdamker closed this Jul 29, 2025
@hdamker hdamker reopened this Jul 29, 2025
@hdamker
Copy link
Contributor

hdamker commented Jul 29, 2025

@stroncoso-quobis seems to work. @deepakjaiswal1 please approve as well again so that we can merge here.

@hdamker
Copy link
Contributor

hdamker commented Jul 29, 2025

the branch protection on *release* wasn't necessary here at all (legacy, but might get reintroduced with new release workflow), but was also not the root cause (your "release" branch is not in the repository).

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.

Your situation—where every PR approval on GitHub is immediately dismissed as "stale" with no visible action in between, making merge impossible, even though the fork branch is 20 commits ahead—strongly suggests a branch protection rule is causing this automatic review dismissal.

Likely Cause

  • "Dismiss stale pull request approvals when new commits are pushed" is enabled in your repo's branch protection rules.
    • With this enabled, any push (including rebases, force-pushes, or even some branch sync operations) to the PR's source branch causes all previous approvals to be marked stale and dismissed[1][2][3].
    • This sometimes happens even if there are no meaningful file changes, such as after rebasing or force-pushing unmodified commits[1][2][4].

This is why the UI says e.g. “<author> dismissed <approver>’s stale review” at exactly the same time as the approval: GitHub does this automatically—it’s not user action[1].

Why You Can't Merge

  • When all reviews are dismissed, and your repo requires reviews, GitHub blocks the merge, waiting for a fresh approval[5][6].
  • Even if the branch seems “clean” and just ahead, rebases or other actions can still trigger this, especially if protection is on both the upstream/main and PR branch[7].

How to Fix

  1. Check Branch Protection Settings:

    • Go to: SettingsBranchesBranch protection rules → Edit the relevant rule.
    • Uncheck "Dismiss stale pull request approvals when new commits are pushed"[1][2][7][3].
  2. Alternatively, use the setting “Require approval of the most recent reviewable push”:

    • This is a newer/alternative setting with a subtly different behavior: it only requires at least one reviewer to approve the latest push, but does not auto-dismiss prior reviews with every commit[7][8].
    • You can enable this and disable "Dismiss stale approvals" for a more flexible workflow.
  3. Temporary Workarounds:

    • Try closing and re-opening the PR. Some users report this temporarily resets the review state and may allow merging[9][6].
    • If the branch is ahead due to unique commits, ensure it is up-to-date with upstream/main and, if needed, squash or rebase-interactively to reduce history/manipulations.

Steps:

1. **Repository Settings** > **Branches**
2. Locate your branch protection rule for the target branch (e.g., main)
3. Click **Edit**
4. Uncheck: "Dismiss stale pull request approvals when new commits are pushed"
5. Save changes
6. Re-approve and merge your PR as usual

Additional Notes

  • These settings are especially problematic for forks, rebased branches, and PRs that see ongoing force-pushes or rebased/clean-up commits—these can all cause spurious dismissals.
  • Make sure your policy balances necessary code review stringency with workflow flexibility.

By disabling or adjusting the relevant branch protection rule, you can prevent these automatic dismissals and allow PR reviews to persist through non-significant pushes, making merging possible again[1][2][7].

Sources
[1] Automatic dismissal of reviews via push is misrepresented in UI #1157 isaacs/github#1157
[2] Intelligent 'Dismiss stale approvals when new commits are pushed' https://github.com/orgs/community/discussions/12876
[3] Github Code Reviews: Automatically Reset and Rerequest ... https://stackoverflow.com/questions/69227774/github-code-reviews-automatically-reset-and-rerequest-approvals-after-changes
[4] GitHub Dismiss Stale approvals and rebasing https://stackoverflow.com/questions/63694736/github-dismiss-stale-approvals-and-rebasing
[5] Reviewers panel on pull request page should reflect ... - GitHub https://github.com/orgs/community/discussions/162757
[6] Can not merge pull requests after approval due to The ... - GitHub https://github.com/orgs/community/discussions/58535
[7] Automatic change of base branch dismisses PR reviews #57513 https://github.com/orgs/community/discussions/57513
[8] What's the difference between `Dismiss stale pull request ... https://github.com/orgs/community/discussions/109549
[9] Github PR: Approval needed after approval has already ... https://stackoverflow.com/questions/78550658/github-pr-approval-needed-after-approval-has-already-been-given
[10] Automatically dismiss approvals when mark Draft PRs as "Ready for ... isaacs/github#1992
[11] Dismissing a pull request review - GitHub Docs https://docs.github.com/articles/dismissing-a-pull-request-review
[12] "Dismiss Review": The Underrated GitHub Feature You're Probably Not Using https://dev.to/aaronsteers/the-most-underrated-github-feature-youre-probably-not-using-dismiss-review-1h0e
[13] git: Your branch is ahead by X commits - Stack Overflow https://stackoverflow.com/questions/2432579/git-your-branch-is-ahead-by-x-commits
[14] Is there a way to filter pull requests where my review is stale/dismissed? https://stackoverflow.com/questions/75252312/is-there-a-way-to-filter-pull-requests-where-my-review-is-stale-dismissed
[15] Working on another branch while a review is ongoing on master https://groups.google.com/g/repo-discuss/c/vDtRm0T66M0/m/6Hr0M9BWu2oJ
[16] Dismiss stale pull requests when new commits are pushed https://jhall.io/archive/2024/02/19/dismiss-stale-pull-requests-when-new-commits-are-pushed/
[17] Disable "had recent pushes" banner for some branches #35200 https://github.com/orgs/community/discussions/35200
[18] Allow disabling "Dismiss stale pull request approvals when new ... https://github.com/orgs/community/discussions/77578
[19] Repository Branches and Commits preview feedback #70668 - GitHub https://github.com/orgs/community/discussions/70668
[20] Approving a pull request with required reviews - GitHub Docs https://docs.github.com/articles/approving-a-pull-request-with-required-reviews

@stroncoso-quobis stroncoso-quobis merged commit c8bd612 into camaraproject:main Jul 30, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

subproject management Indicating issues with subproject repository or release management process

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scope for Fall25 WebRTC releases (in preparation)

4 participants