fix(variables): add noop mode support#926
fix(variables): add noop mode support#926klutchell wants to merge 1 commit 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.
klutchell
force-pushed
the
balena/noop-vars
branch
2 times, most recently
from
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.
- 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>
klutchell
force-pushed
the
balena/noop-vars
branch
from
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.
| if (this.nop) { | ||
| return new NopCommand( | ||
| this.constructor.name, | ||
| this.repo, | ||
| null, | ||
| `Add variable ${variable.name}` | ||
| ) | ||
| } |
| this.log.debug(`Updating a repo var existing params ${JSON.stringify(existing)} and new ${JSON.stringify(variables)}`) | ||
| existing = _.castArray(existing) | ||
| variables = _.castArray(variables) | ||
| const changed = this.getChanged(existing, variables) | ||
|
|
||
| if (changed) { | ||
| const nopCommands = [] | ||
| let existingVariables = [...existing] |
Fixes: #925