Skip to content

Mark all existing options as non-updateable - #377

Merged
sirzooro merged 1 commit into
pion:mainfrom
sirzooro:options_not_updateable
Sep 6, 2026
Merged

Mark all existing options as non-updateable#377
sirzooro merged 1 commit into
pion:mainfrom
sirzooro:options_not_updateable

Conversation

@sirzooro

Copy link
Copy Markdown
Contributor

Initial changes to allow changing selected options at runtime. This is needed for Cryptex implementation.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a constructed flag on Context and uses it to make all existing ContextOptions reject mutation after context construction, establishing a baseline for future “selectively updatable options” work (notably for Cryptex).

Changes:

  • Add Context.constructed and set it during CreateContext to gate option updates.
  • Make all existing options in option.go return ErrContextOptionNotUpdatable after construction.
  • Add/adjust tests to validate the new non-updatability behavior and update a test helper to mark contexts as constructed.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
context.go Adds the constructed flag to Context and sets it during CreateContext.
errors.go Introduces ErrContextOptionNotUpdatable for post-construction option mutation attempts.
option.go Adds constructed-state checks to all existing ContextOptions.
option_test.go New test coverage for option “set during construction only” behavior.
srtp_cipher_utils_test.go Marks test-created contexts as constructed to align with new option gating.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread option.go
Comment thread option_test.go Outdated
Comment thread option_test.go Outdated
Comment thread option_test.go Outdated
Comment thread option_test.go Outdated
Comment thread option_test.go Outdated
Comment thread option_test.go Outdated
Comment thread context.go
@sirzooro
sirzooro force-pushed the options_not_updateable branch from 39b05fe to 39575de Compare September 2, 2026 04:45
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.23%. Comparing base (df5f1a8) to head (1842d60).

Files with missing lines Patch % Lines
option.go 92.85% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
+ Coverage   84.06%   84.23%   +0.17%     
==========================================
  Files          19       19              
  Lines        1512     1541      +29     
==========================================
+ Hits         1271     1298      +27     
- Misses        134      135       +1     
- Partials      107      108       +1     
Flag Coverage Δ
go 84.23% <93.10%> (+0.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sirzooro
sirzooro force-pushed the options_not_updateable branch from 39575de to 51b426d Compare September 2, 2026 05:59
Initial changes to allow changing selected options at runtime. This is
needed for Cryptex implementation.
@sirzooro
sirzooro force-pushed the options_not_updateable branch from 51b426d to 1842d60 Compare September 6, 2026 06:03
@sirzooro
sirzooro requested a review from JoTurk September 6, 2026 06:07
@sirzooro
sirzooro merged commit 94b53a4 into pion:main Sep 6, 2026
18 checks passed
@sirzooro
sirzooro deleted the options_not_updateable branch September 6, 2026 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants