Repository navigation
fix(uninstall): correct the dry-run footer and stop answering --force with --force-dev-binaries - #916
Draft
jussielo-amd wants to merge 2 commits into
Conversation
… with --force-dev-binaries A dry run ended with "Choose Review uninstall to approve removal.", a control the command line does not have, and it printed that even when there was nothing to remove. A person at a terminal now gets "run it again without --dry-run" when there is something to remove, and nothing when there is not. Output that is captured (the dashboard's /uninstall and the daemon's rocm_command tool run the dry run as a subprocess) names no next step, since the real step belongs to whatever drives it. The assistant's preview has no flag to offer, so it says removal needs approval. The three wordings come from one audience value, chosen from whether the command has a terminal, rather than from strings that can drift. A mistyped --force was answered with "a similar argument exists: --force-dev-binaries", which removes the binaries of a development build, and so were near misses of it. clap ranks every declared flag by name, so --force is now declared, hidden, and refused with a pointer to --yes before anything is planned; near misses land on it instead of on the destructive flag, and a typo of the whole flag is still met with that flag. rocm uninstall --help now ends with examples that start with a dry run. Scenarios uninstall-07 to uninstall-11 cover the footer at a terminal (run again as it says, answering the prompt), outside one, with an empty plan, the --force cases, and the help examples. Every uninstall step now refuses to run without --keep-binaries. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
The step that finds the `EXAMPLES:` block in `rocm uninstall --help` used `map(..).unwrap_or_else(..)`, which the workspace's pedantic lints reject, so the clippy job and both Windows build jobs failed to compile the e2e harness. `map_or_else` keeps the same panic message and behavior. Signed-off-by: Jussi Elo <jussi.elo@amd.com>
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
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.
Depends on #907. This pull request is stacked on it: its base is that pull request's branch, so the diff below is only this change, and it can be retargeted to
mainonce #907 merges.Summary
A dry run of
rocm uninstallended with "Choose Review uninstall to approve removal.", a control the command line does not have, and it printed that line even when there was nothing to remove. A person at a terminal now gets "run it again without --dry-run" when there is something to remove, and nothing when there is not. Output that is captured (the dashboard's/uninstalland the daemon'srocm_commandtool run the dry run as a subprocess) names no next step, since the real step belongs to whatever drives it. The assistant's uninstall preview shares the renderer and has no flag to offer, so it says removal needs approval. The three wordings come from one audience value, chosen from whether the command has a terminal.A mistyped
--forcewas answered with "a similar argument exists:--force-dev-binaries", which removes the binaries of a development build, and so were near misses of it. clap ranks every declared flag by name, so--forceis now declared, hidden, and refused with a pointer to--yesbefore anything is planned. Near misses such as--forcland on it instead of the destructive flag, and a typo of the whole flag is still met with that flag.rocm uninstall --helpnow ends with examples that start with a dry run.Fixes #788.
Tests
Scenarios uninstall-07 to uninstall-11 cover the footer at a terminal (run again as it says, answering the prompt, and assert the folder is gone), outside a terminal, with an empty plan, the
--forcecases, and the help examples. The terminal scenarios use a pseudo-terminal and run on the Linux lane; the others are not OS-gated and run on the mock lane on every PR. Unit tests cover the footer wording per audience, and I checked that each new test and scenario goes red when the behavior it covers is broken.The scenario numbers overlap with draft PR #569, which also adds scenarios to
uninstall.feature, so one of the two will need renumbering when the other lands.Run locally:
cargo xtask e2e -- -n "uninstall-"(11 of 11), therocmunit tests, clippy with-D warningsforrocmande2e-cucumber, andcargo fmt --check. Not run: the full workspace gate, the pre-push hooks, Windows (the pseudo-terminal scenarios are Linux-only), and clippy forrocm-dash-tuiandrocmd, which this change does not edit. CI is the authoritative check for the rest.