Skip to content

docs: fix assert artifact help text on flag mutual exclusivity#768

Merged
dangrondahl merged 3 commits intomainfrom
fix-assert-artifact-docs
Apr 9, 2026
Merged

docs: fix assert artifact help text on flag mutual exclusivity#768
dangrondahl merged 3 commits intomainfrom
fix-assert-artifact-docs

Conversation

@dangrondahl
Copy link
Copy Markdown
Contributor

Summary

The help text for kosli assert artifact incorrectly stated all four modes (environment, policy, flow, all-flows) were mutually exclusive.

After investigating the server-side implementation (src/fastapi_app/common/asserts.py), only --environment and --policy are mutually exclusive (server returns 400 if both are provided). --flow is an independent filter that can be combined with either.

Updated the help text to accurately describe the three assertion modes and clarify that --flow narrows the artifact lookup scope rather than being a separate exclusive mode.

The help text incorrectly stated all four modes were mutually exclusive.
In reality only --environment and --policy are mutually exclusive.
--flow is an independent filter that can be combined with either.
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Claude finished @dangrondahl's task in 1m 43s —— View job


Review of PR #768

  • Run static analysis (go build, go vet, go mod tidy)
  • Review code changes
  • Post review summary

Static Analysis

All checks pass — go build ./..., go vet ./..., and go mod tidy produce no issues.


Code Review

Help text (assertArtifact.go:15-32) — The rewritten description is a clear improvement. The old text incorrectly listed four mutually exclusive modes; the new text accurately describes three assertion scopes and correctly explains that --flow is an independent filter. The structured layout (numbered modes, then clarifying notes) reads well.

Client-side enforcement (assertArtifact.go:111)cmd.MarkFlagsMutuallyExclusive("environment", "policy") is the right Cobra idiom. This gives users a fast, clear error without a server round-trip. This is the first use of MarkFlagsMutuallyExclusive in this codebase, which is fine — it's a standard Cobra API.

Test update (assertArtifact_test.go:188) — The golden string for test case 15 correctly matches Cobra's MarkFlagsMutuallyExclusive error format. Good catch following up on the review feedback.

What's done well

  • The PR description shows solid investigation of the server-side behavior before changing docs — the change is grounded in actual API semantics, not guesswork.
  • The three commits are logically separated (docs fix → behavior change → test fix), making the history easy to follow.
  • Client-side validation is a genuine UX improvement — fail fast with a clear message.

Summary

Clean, well-scoped PR. No issues found. LGTM.

Use Cobra's MarkFlagsMutuallyExclusive to give users a fast error
message without a server round-trip.
@dangrondahl
Copy link
Copy Markdown
Contributor Author

Added cmd.MarkFlagsMutuallyExclusive("environment", "policy") in commit b21572a. Users now get a fast client-side error without a server round-trip:

$ kosli assert artifact --environment foo --policy bar --fingerprint 184c...9cc0 --org test --api-token test
Error: if any flags in the group [environment policy] are set none of the others can be; [environment policy] were all set


addFingerprintFlags(cmd, o.fingerprintOptions)
addDryRunFlag(cmd)
cmd.MarkFlagsMutuallyExclusive("environment", "policy")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: This client-side enforcement changes the error message, but the existing test case "15 providing both --environment and --polices fails" in assertArtifact_test.go:184-189 still expects the old server-side error:

"Error: Cannot specify both 'environment_name' and 'policy_name' at the same time\n"

With MarkFlagsMutuallyExclusive, Cobra will now emit:

"Error: if any flags in the group [environment policy] are set none of the others can be; [environment policy] were all set"

The golden string in the test needs to be updated to match the new client-side message, otherwise this test will fail.

Fix this →

@dangrondahl dangrondahl enabled auto-merge (squash) April 9, 2026 07:33
dangrondahl added a commit to kosli-dev/docs that referenced this pull request Apr 9, 2026
…133)

## Summary

The assertion scopes table incorrectly listed four mutually exclusive
modes. After investigating the server implementation
(`src/fastapi_app/common/asserts.py`), only `--environment` and
`--policy` are mutually exclusive (server returns 400 if both are
provided). `--flow` is an independent filter that narrows the artifact
lookup scope.

Updated to describe three assertion modes with a note that `--flow` can
be combined with any of them.

Companion CLI PR: kosli-dev/cli#768
@dangrondahl dangrondahl merged commit aa2066e into main Apr 9, 2026
20 checks passed
@dangrondahl dangrondahl deleted the fix-assert-artifact-docs branch April 9, 2026 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants