fix(cli): --yes no longer overrides --no-cleanup precedence#86
Merged
Conversation
The `if yes { ... }` block in runUpgrade ran unconditionally AFTER
the `switch` that was supposed to make `--no-cleanup` win. It
flipped `NonInteractive` back to `false` and set
`AutoDeleteAll=true`, so `nodeup upgrade --yes --no-cleanup`
silently deleted every eligible old Node.js version with zero
confirmation of any kind — the exact opposite of `--no-cleanup`'s
documented "never prompt, never delete" contract.
This is a real-world hazard: CI scripts habitually pass
`-y`/`--yes` to every CLI, and a maintainer's npm/nodeup
playbook running such a script could lose Node installs it never
asked to remove.
The cleanup-toggle resolution is factored into a pure helper
`resolveCleanupConfig(noCleanup, autoCleanup, yes, versions, cfg)`
in cleanup.go, and the `--yes` block is guarded with
`&& !noCleanup` so `--no-cleanup` wins on every flag combination.
New tests in cleanup_config_test.go pin the precedence table:
- TestResolveCleanupConfig_NoCleanupBeatsYes (the bug)
- TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup (CI
scripts still get the auto-confirm they expect)
- TestResolveCleanupConfig_CleanupFlagSetsAutoDeleteAll /
TestResolveCleanupConfig_ConfigAutoSetsAutoDeleteAll (each input
knob individually)
- TestResolveCleanupConfig_PrefilteredPropagates (--cleanup-version
flows through unchanged)
- TestResolveCleanupConfig_DefaultsAreInteractive /
TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion
- TestResolveCleanupConfig_NoCleanupBeatsEverythingElse — negative
matrix covering every knob `--no-cleanup` must beat, so a future
regression that re-introduces the bug (or its sibling in a new
knob) is caught at the helper level rather than via a full
end-to-end upgrade run.
Refs #57
|
✔️ 9ad9d02 - Conventional commits check succeeded. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
nodeup upgrade --yes --no-cleanupsilently auto-deleted every eligible old Node.js version. Theif yes { ... }block inrunUpgraderan unconditionally after theswitchthat was supposed to make--no-cleanupwin — it flippedNonInteractiveback tofalseand setAutoDeleteAll=true, regardless ofnoCleanup. That contradicts the documented contract of--no-cleanup("never prompt, never delete") and is a real-world hazard for any CI script that habitually passes-y/--yes.This PR factors the cleanup-toggle resolution into a pure helper
resolveCleanupConfig(noCleanup, autoCleanup, yes, versions, cfg)incleanup.go, and guards the--yesblock with&& !noCleanupso--no-cleanupwins on every flag combination.Linked issues
Fixes #57
Type of change
fix— bug fix (PATCH bump)feat— new feature (MINOR bump)refactor— no behavior changeperf— performance improvementdocs— documentation onlytest— tests onlychore— maintenance / dependency bumpci— CI/CD onlybuild— build system onlyChecklist
feat(scope): subject)make cilocally and it passes (tidy/fmt/vet/test all green;make lintfails locally with the documented build(lint): .golangci.yml v1 schema incompatible with golangci-lint v2 (brew installs v2, make lint fails to even parse config) #62 golangci v1/v2 schema issue — CI uses the pinned v1.64.8 which is unaffected)docs/, inline godoc)Screenshots / output
Pre-fix behavior:
Post-fix behavior:
--yesalone (the CI-friendly behavior the bug tried to defend) is unchanged:Files touched
internal/cli/cleanup.go— new helperresolveCleanupConfig(noCleanup, autoCleanup, yes, cleanupVersions, cfg config.CleanupConfig) cleanupConfig. Doc comment carries the precedence table and references bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57. New import:internal/config.internal/cli/upgrade.go— replaces the inline 30-line resolution block with a single call toresolveCleanupConfig(...). Drops the_ = yesplaceholder and the now-redundant precedence comment.internal/cli/cleanup_config_test.go— new file. 7 top-level test functions / 9 sub-tests pinning the precedence table end-to-end:TestResolveCleanupConfig_NoCleanupBeatsYes— the regression test for bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57.TestResolveCleanupConfig_YesAutoDeletesWithoutNoCleanup— pins that CI scripts passing-ystill get auto-confirm.TestResolveCleanupConfig_CleanupFlagSetsAutoDeleteAll,TestResolveCleanupConfig_ConfigAutoSetsAutoDeleteAll— each input knob individually.TestResolveCleanupConfig_PrefilteredPropagates—--cleanup-versionflows through unchanged.TestResolveCleanupConfig_DefaultsAreInteractive,TestResolveCleanupConfig_CfgPromptFalseSkipsPerVersion— default andcfg.Cleanup.Prompt=falsepaths.TestResolveCleanupConfig_NoCleanupBeatsEverythingElse— negative-path matrix (4 sub-tests) covering every knob--no-cleanupmust beat, so a future regression that re-introduces the bug (or its sibling in a new knob) is caught at the helper level rather than via a full end-to-end upgrade run.CHANGELOG.md—### Fixedentry under[Unreleased]documenting the change with the bug(cli): --yes silently overrides --no-cleanup, auto-deletes with zero confirmation #57 reference.