feat(cli): enforce required policy flags from man docs - #3840
Open
alkalescent wants to merge 3 commits into
Open
feat(cli): enforce required policy flags from man docs#3840alkalescent wants to merge 3 commits into
alkalescent wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (42)
💤 Files with no reviewable changes (9)
📝 WalkthroughWalkthroughPolicy command documentation now defines required flags. ChangesPolicy CLI required flags
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
alkalescent
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
The man-doc frontmatter already declared `required: true` on many policy command flags, but `man.DocFlag` never parsed it, so required-ness was only validated at runtime via GetRequiredString/GetRequiredID. Nothing marked the flags required at the cobra layer, so tools generated from the command tree (e.g. MCP) could not advertise which inputs are mandatory. - Add `Required` to `man.DocFlag` and a `Doc.MarkRequiredFlags()` method (mirrors MarkSensitiveFlags; skips flags not registered on the command). - Apply it to every policy command via a sweep in policy.InitCommands(). - Reconcile the doc `required: true` set with the run functions: add it where a flag is unconditionally required but the doc omitted it, and remove it where the doc over-claimed (deprecated no-op flags, "one of" groups, and paginated limit/offset that default rather than enforce).
Mark the unconditionally-required multi-value flags that the handlers reject when empty but the docs left optional, so cobra enforces them and generated schemas advertise them: - subject-mappings create --action (handler errors on zero actions) - dynamic-value-mappings create --action (handler errors on zero actions) - resource-mappings create --terms (GetStringSlice Min: 1) The subject-condition-set "one of" groups stay unmarked (genuinely conditional).
alkalescent
force-pushed
the
feat/policy-required-flags-from-docs
branch
from
August 6, 2026 05:12
bb487b8 to
6dfcb9d
Compare
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
Contributor
The policy required-flag sweep makes cobra reject missing required flags with "required flag(s) \"<flag>\" not set" before the run function executes, replacing the previous custom messages. Update the e2e BATS assertions across policy commands to expect the cobra output, including sibling sub-cases BATS did not reach. Assertions for flags that remain conditionally required or governed by one-of validation (e.g. get/base-key --kas, subject-mapping SCS) are unchanged. Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
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
Policy command flags declared
required: truein their man-doc frontmatter werenever enforced at the cobra layer.
man.DocFlagdid not parse therequiredkey, so required-ness was only checked at runtime via
GetRequiredString/GetRequiredID. As a result, generated tooling that readsthe command tree could not advertise which inputs are
mandatory, and shell completion did not mark them required.
Requiredtoman.DocFlagand aDoc.MarkRequiredFlags()method,mirroring the existing
MarkSensitiveFlags. It skips flags declared requiredin the doc but not registered on the command (subcommand / shared injector),
so a central sweep is safe.
policy.InitCommands(), once all flags are registered.required: trueset with its runfunction so the declared requirements match actual behavior:
required: truewhere a flag is unconditionally required in Gobut the doc omitted it (e.g.
namespaces get --id,resource-mappingsandresource-mapping-groupsid/create flags,attributes valuesids,dynamic-value-mappings createattribute/selector/operator).required: truewhere the doc over-claimed: deprecated no-opflags (
kas-grants assign/unassign), mutually-exclusive "one of" groups(
subject-condition-sets create --subject-sets,attributes create --value, kas key--kas), and paginatedlimit/offsetthat fall back toa default rather than being enforced.
Behavior
Before:
otdfctl policy namespaces getfailed at runtime with a custom error.After: cobra reports
required flag(s) "id" not setand the requirement isvisible in
--help, completion, and generated tool schemas. Commands whose docsover-claimed (e.g.
kas-registry key listwith default pagination) are no longerblocked.
Testing
go build ./...go test ./otdfctl/pkg/man/... ./otdfctl/cmd/...(new tests cover parsing andmarking; existing policy command tests pass)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Tests