Honor --key in bundle generate dashboard and make lookup flags mutually exclusive#5492
Open
simonfaltum wants to merge 3 commits into
Open
Honor --key in bundle generate dashboard and make lookup flags mutually exclusive#5492simonfaltum wants to merge 3 commits into
simonfaltum wants to merge 3 commits into
Conversation
The dashboard generator always derived the resource key from the display name even though --key is documented in its help text and honored by the other generate commands. Also enforce that only one of --existing-path, --existing-id, and --resource is provided instead of silently preferring one over the others. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 7e18efc
28 interesting tests: 15 SKIP, 7 KNOWN, 6 flaky
Top 50 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Comment on lines
+553
to
+557
| cmd.MarkFlagsMutuallyExclusive( | ||
| "existing-path", | ||
| "existing-id", | ||
| "resource", | ||
| ) |
Contributor
There was a problem hiding this comment.
What would happen before this? Based on a quick view it seems that previously there was some precedence between flags where some would get ignored. Not good.
However, now we'll throw an error. Is this a breaking changes? What if people are using multiple flags somewhere and it just works for them?
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
3 participants
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.
Why
Found during a full-repo review of the CLI.
bundle generate dashboardsilently ignored the--keyflag, even though its own help text documents it and every other generate command (job, pipeline, alert) honors it. In addition, the three lookup flags (--existing-path,--existing-id,--resource) could be combined freely, with one silently shadowing the others, so users got no signal that part of their invocation was ignored.Changes
Before, the dashboard resource key was always derived from the dashboard's display name and conflicting lookup flags were silently accepted; now
--keytakes precedence over the derived key and passing more than one lookup flag fails with a validation error.cmd/bundle/generate/dashboard.go: readcmd.Flag("key")and fall back to the normalized display name when it is empty, matching the pattern used byjob.go. Thekeyflag is a persistent flag on the parentgeneratecommand.cmd/bundle/generate/dashboard.go: mark--existing-path,--existing-id, and--resourceas mutually exclusive. Combined with the existingMarkFlagsOneRequired, exactly one lookup flag is now required, which is what the existing comment already promised.Test plan
TestDashboard_LookupFlagsAreMutuallyExclusivecovering all three conflicting flag pairs (go test ./cmd/bundle/generate/)acceptance/bundle/generate/dashboard_existing_path_nominalacceptance test with a--key custom_keyinvocation; verified the generated config uses the custom key (go test ./acceptance -run TestAccept/bundle/generate/dashboard_existing_path_nominal, both engine variants pass)./task fmt-q,./task lint-q, and./task checkspassThis pull request and its description were written by Isaac.