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