Conversation
There was a problem hiding this comment.
Pull request overview
Follow-up to #2348 to refine how the refresh controller tracks snapd change IDs initiated by the current server process, aligning implementation and tests with the intended “only restart for our own change IDs” behavior.
Changes:
- Switched
RefreshController.initiated_changesfromlist[str]toset[str]and updated insertion accordingly. - Updated refresh progress tests to use string change IDs and set-based
our_change_ids.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
subiquity/server/controllers/refresh.py |
Tracks initiated snapd change IDs as a set and uses .add() when starting refresh. |
subiquity/server/controllers/tests/test_refresh.py |
Adjusts parameterized test cases to use string IDs and set inputs to match controller behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
ogayot
force-pushed
the
refresh-no-restart-address-comments
branch
from
d2d5f0f to
ad259a5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # List of snapd change IDs for refresh operations that we initiated. | ||
| # We use a list because refresh operations can fail and be restarted. | ||
| # Set of snapd change IDs for refresh operations that we initiated. | ||
| # We use a set because refresh operations can fail and be restarted. |
There was a problem hiding this comment.
The rationale in this comment doesn’t match the code change: using a set isn’t related to refresh operations being restarted (a list would also allow multiple change IDs). The concrete reason to prefer a set here is fast membership checks (change_id in initiated_changes) and uniqueness. Please update the comment so it accurately describes why a set is used.
| # We use a set because refresh operations can fail and be restarted. | |
| # We use a set to ensure uniqueness and for fast membership checks | |
| # when verifying whether a given change_id is one of ours. |
There was a problem hiding this comment.
Yes, but it's not about the container being a set or a list. It is about storing multiple values.
| @parameterized.expand( | ||
| ( | ||
| # Here it's our own change | ||
| (7, TaskStatus.DO, [7], False), | ||
| (7, TaskStatus.DOING, [7], False), | ||
| (7, TaskStatus.DONE, [7], True), | ||
| (7, TaskStatus.DONE, [6, 7], True), | ||
| ("7", TaskStatus.DO, {"7"}, False), | ||
| ("7", TaskStatus.DOING, {"7"}, False), | ||
| ("7", TaskStatus.DONE, {"7"}, True), | ||
| ("7", TaskStatus.DONE, {"6", "7"}, True), | ||
| # And here, it's somebody else's | ||
| (7, TaskStatus.DO, [], False), | ||
| (7, TaskStatus.DOING, [], False), | ||
| (7, TaskStatus.DONE, [], False), | ||
| (7, TaskStatus.DONE, [1, 2, 3], False), | ||
| ("7", TaskStatus.DO, set(), False), | ||
| ("7", TaskStatus.DOING, set(), False), | ||
| ("7", TaskStatus.DONE, set(), False), | ||
| ("7", TaskStatus.DONE, {"1", "2", "3"}, False), |
There was a problem hiding this comment.
These parameterized cases now include set literals (e.g. {"6", "7"}); the string representation/order of a set is not stable across runs due to hash randomization, and many parameterized runners derive the generated test name from repr(...). Consider passing an ordered collection (e.g. tuple/list) and converting to set inside the test, or using frozenset(sorted(...)), to keep generated test names deterministic.
Follow up PR for #2348