fix(variables): add noop mode support#926
fix(variables): add noop mode support#926klutchell wants to merge 2 commits intogithub:main-enterprisefrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds proper noop (dry-run) behavior to the variables plugin so repository variables are not mutated when nop: true, addressing safe-settings issue #925.
Changes:
- Added
NopCommand-based noop handling toVariables.add(),Variables.remove(), andVariables.update()to avoid mutating API calls in dry-run mode. - Expanded unit tests to cover noop behavior for
add/remove/updateand adjusted test setup to passnopinto the plugin.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
lib/plugins/variables.js |
Adds noop checks and returns NopCommand(s) instead of performing POST/PATCH/DELETE when nop is enabled. |
test/unit/lib/plugins/variables.test.js |
Adds unit tests validating noop behavior for variables operations and updates test harness to pass nop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0d2cc5e to
723c3be
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.
723c3be to
f2eea33
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 no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@decyjphr this one should be good to go! |
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.
Comments suppressed due to low confidence (1)
lib/plugins/variables.js:178
- In noop mode,
update()currently returnsnopCommands.length === 1 ? nopCommands[0] : nopCommands, which means it can return[]whenchangedis true but no concrete operations are generated. That bubbles up throughDiffable.sync()and can produce non-NopCommanditems in the returned results array. Consider returningundefined/nullwhennopCommands.length === 0(or ensuringchangedis only true when an operation will be emitted).
if (this.nop) {
return nopCommands.length === 1 ? nopCommands[0] : nopCommands
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
f2eea33 to
4ca8912
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 no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add noop mode support to Variables plugin add/remove/update methods - Return NopCommand instead of making API calls when nop=true - Add comprehensive tests for noop mode behavior Signed-off-by: Kyle Harding <kyle@balena.io>
Refactor Variables plugin to match the single-item Diffable contract used by labels, milestones, and other plugins. The previous update() reimplemented sync() logic internally; now each method handles one item and lets Diffable.sync() orchestrate iteration. - Simplify update() from 90-line array-diffing to single-item PATCH - Simplify changed() from JSON.stringify comparison to value check - Remove getChanged(), lodash dependency, .then(res=>res) no-ops - Match labels.js nop return pattern: Promise.resolve([NopCommand]) - Fix inconsistent toUpperCase() between add/remove/update - Let errors propagate to Diffable.sync() instead of swallowing - Normalize find() to strip API metadata fields (created_at, etc.) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Kyle Harding <kyle@balena.io>
4ca8912 to
e43d46f
Compare
|
@decyjphr conflicts resolved |
Fixes: #925
Refactor Variables plugin to match the single-item Diffable contract
used by labels, milestones, and other plugins. The previous update()
reimplemented sync() logic internally; now each method handles one item
and lets Diffable.sync() orchestrate iteration.