feat: Add --show-policy-docs-link flag (default: true)#3173
feat: Add --show-policy-docs-link flag (default: true)#3173dheerajodha wants to merge 1 commit intoconforma:mainfrom
Conversation
Review Summary by QodoAdd --show-policy-docs-link flag to control policy documentation visibility WalkthroughsDescription• Add --show-policy-docs-link persistent flag to control policy documentation link visibility • Flag defaults to true, maintaining backward compatibility with existing behavior • Propagate flag through all validate subcommands (image, input, vsa) and report generation • Update templates and format options to conditionally render documentation link based on flag Diagramflowchart LR
A["validate command"] -->|"persistent flag"| B["--show-policy-docs-link<br/>default: true"]
B -->|"inherited by"| C["image subcommand"]
B -->|"inherited by"| D["input subcommand"]
B -->|"inherited by"| E["vsa subcommand"]
C -->|"passes to"| F["ReportData"]
D -->|"passes to"| F
E -->|"passes to"| F
F -->|"controls rendering"| G["Documentation link<br/>in output"]
File Changes1. cmd/validate/validate.go
|
Code Review by Qodo
1. Fallback link always hidden
|
📝 WalkthroughWalkthroughThis pull request adds a new CLI flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
I'm thinking if the flag name is too long 🤔 |
| {{- end -}} | ||
|
|
||
| {{- if or (gt $t.Failures 0) (and (gt $t.Warnings 0) $r.ShowWarnings) -}} | ||
| {{- if and $r.ShowPolicyDocsLink (or (gt $t.Failures 0) (and (gt $t.Warnings 0) $r.ShowWarnings)) -}} |
There was a problem hiding this comment.
1. Fallback link always hidden 🐞 Bug ✓ Correctness
In VSA text output, the fallback validate-image section omits the policy documentation link even when --show-policy-docs-link=true (default), because the fallback text capture builds format.Options without ShowPolicyDocsLink and applicationsnapshot.Report.WriteAll overwrites the report setting with the default false option. The new template guard added in this PR then prevents the link from rendering in that fallback section.
Agent Prompt
### Issue description
VSA text output captures fallback validate-image text via `captureFallbackText`. That function constructs `format.Options` without `ShowPolicyDocsLink`, causing `applicationsnapshot.Report.WriteAll()` to overwrite `Report.ShowPolicyDocsLink` to `false`. Since the text template now requires `ShowPolicyDocsLink` to print the documentation link, the fallback section never shows the link even when `--show-policy-docs-link=true`.
### Issue Context
- The fallback report data correctly includes `ShowPolicyDocsLink`, but it is later overridden by target options.
- `applicationsnapshot.Report.WriteAll()` always applies `target.Options` to the report before rendering.
### Fix Focus Areas
- cmd/validate/vsa.go[1432-1446]
Suggested change: include `ShowPolicyDocsLink: fallbackReport.ShowPolicyDocsLink` in `formatOpts` inside `captureFallbackText`, and consider adding/adjusting a test that asserts the fallback section contains/omits the docs link based on the flag.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/input/report.go (1)
101-125:input.Reportstill can't honor per-output overrides for this new flag.
ShowPolicyDocsLinkis now fixed atNewReport(...)construction time, butWriteAllnever appliestarget.Optionsbefore rendering. So theec validate inputpath still won't honor output-scoped?show-...overrides the wayapplicationsnapshot.Reportdoes.Possible follow-up
+func (r *Report) applyOptions(opts format.Options) { + r.ShowSuccesses = opts.ShowSuccesses + r.ShowWarnings = opts.ShowWarnings + r.ShowPolicyDocsLink = opts.ShowPolicyDocsLink +} + // WriteAll writes the report to all the given targets. func (r Report) WriteAll(targets []string, p format.TargetParser) (allErrors error) { if len(targets) == 0 { targets = append(targets, Text) } for _, targetName := range targets { target, err := p.Parse(targetName) if err != nil { allErrors = errors.Join(allErrors, err) continue } + + r.applyOptions(target.Options) data, err := r.toFormat(target.Format) if err != nil { allErrors = errors.Join(allErrors, err) continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/input/report.go` around lines 101 - 125, NewReport currently fixes ShowPolicyDocsLink at construction time so per-output flags in target.Options are ignored; update the flow so NewReport does not permanently bake the show flag (or leaves it unset/default) and ensure WriteAll (the renderer) applies each target.Options prior to rendering—mirror applicationsnapshot.Report's behavior by reading target.Options for ShowPolicyDocsLink inside WriteAll (or setting report.ShowPolicyDocsLink from target.Options just before render) so output-scoped ?show-... overrides are honored.internal/input/report_test.go (1)
39-40: Add a regression test forshowPolicyDocsLink=falseon the input report path.All of the updated
NewReport(...)calls passtruefor the new flag, and none of the text-output assertions cover warnings or violations with the flag disabled. A regression that still prints the docs link forec validate input --show-policy-docs-link=falsewould slip through here.Also applies to: 129-130, 235-236, 321-322, 360-361, 385-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/input/report_test.go` around lines 39 - 40, Add regression tests in internal/input/report_test.go that call NewReport with the showPolicyDocsLink flag set to false (i.e., change or add cases where NewReport(inputs, testPolicy, nil, false, false, true) is used) and assert that the generated text output for warnings/violations does NOT contain the policy docs link; update the other affected test invocations (the similar calls at the other noted spots) to include a corresponding test case with showPolicyDocsLink=false to ensure the docs link is suppressed.internal/applicationsnapshot/report_test.go (1)
1036-1059: Please cover theShowPolicyDocsLink=falsebranch too.These cases prove the link appears when the flag is enabled, but not that it disappears when warnings or violations exist and the flag is disabled. That suppression path is the user-visible behavior this flag adds.
Also applies to: 1065-1087, 1093-1123, 1129-1168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applicationsnapshot/report_test.go` around lines 1036 - 1059, Add a negative test case that asserts the policy docs link is suppressed when ShowPolicyDocsLink is false: construct a Report (same setup as the existing test) with ShowPolicyDocsLink: false and one Component containing a warning (evaluator.Result with "code":"warning.policy"), call generateTextReport(&r) and assert no "https://conforma.dev/docs/policy/" link and that the "For more information about policy issues" message is absent; mirror this for the other similar test blocks referenced (lines around generateTextReport usage) to cover all branches where ShowPolicyDocsLink toggles the link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/validate/validate.go`:
- Line 48: The flag "show-policy-docs-link" is registered on the parent validate
command (validateCmd) but only used by specific subcommands (validate image,
validate input, validate vsa) while ValidatePolicyCmd never reads it; either
move the flag registration off validateCmd and register it only on the
subcommands that use it (the image/input/vsa command builders where flags are
currently read), or add logic in the ValidatePolicyCmd handler to read and honor
the "show-policy-docs-link" persistent flag (use the same flag key) so ec
validate policy --show-policy-docs-link behaves consistently. Ensure you update
the flag registration location(s) and the code path that reads the flag (the
existing readers used by image/input/vsa or the ValidatePolicyCmd) so there are
no unused persistent flags and behavior is consistent.
---
Nitpick comments:
In `@internal/applicationsnapshot/report_test.go`:
- Around line 1036-1059: Add a negative test case that asserts the policy docs
link is suppressed when ShowPolicyDocsLink is false: construct a Report (same
setup as the existing test) with ShowPolicyDocsLink: false and one Component
containing a warning (evaluator.Result with "code":"warning.policy"), call
generateTextReport(&r) and assert no "https://conforma.dev/docs/policy/" link
and that the "For more information about policy issues" message is absent;
mirror this for the other similar test blocks referenced (lines around
generateTextReport usage) to cover all branches where ShowPolicyDocsLink toggles
the link.
In `@internal/input/report_test.go`:
- Around line 39-40: Add regression tests in internal/input/report_test.go that
call NewReport with the showPolicyDocsLink flag set to false (i.e., change or
add cases where NewReport(inputs, testPolicy, nil, false, false, true) is used)
and assert that the generated text output for warnings/violations does NOT
contain the policy docs link; update the other affected test invocations (the
similar calls at the other noted spots) to include a corresponding test case
with showPolicyDocsLink=false to ensure the docs link is suppressed.
In `@internal/input/report.go`:
- Around line 101-125: NewReport currently fixes ShowPolicyDocsLink at
construction time so per-output flags in target.Options are ignored; update the
flow so NewReport does not permanently bake the show flag (or leaves it
unset/default) and ensure WriteAll (the renderer) applies each target.Options
prior to rendering—mirror applicationsnapshot.Report's behavior by reading
target.Options for ShowPolicyDocsLink inside WriteAll (or setting
report.ShowPolicyDocsLink from target.Options just before render) so
output-scoped ?show-... overrides are honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27e72ba8-c638-452c-a969-04b6a8761845
📒 Files selected for processing (17)
cmd/validate/image.gocmd/validate/input.gocmd/validate/validate.gocmd/validate/vsa.godocs/modules/ROOT/pages/ec_validate.adocdocs/modules/ROOT/pages/ec_validate_image.adocdocs/modules/ROOT/pages/ec_validate_input.adocdocs/modules/ROOT/pages/ec_validate_policy.adocdocs/modules/ROOT/pages/ec_validate_vsa.adocinternal/applicationsnapshot/report.gointernal/applicationsnapshot/report_test.gointernal/applicationsnapshot/templates/text_report.tmplinternal/format/target.gointernal/input/report.gointernal/input/report_test.gointernal/input/templates/text_report.tmplinternal/validate/report.go
cmd/validate/validate.go
Outdated
| } | ||
| validateCmd.PersistentFlags().Bool("show-successes", false, "") | ||
| validateCmd.PersistentFlags().Bool("show-warnings", true, "") | ||
| validateCmd.PersistentFlags().Bool("show-policy-docs-link", true, "Show the policy documentation link when there are failures or warnings") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\nValidatePolicyCmd definition:\n'
rg -nC4 --type=go '\bValidatePolicyCmd\s*\(' cmd internal
printf '\nshow-policy-docs-link / ShowPolicyDocsLink references:\n'
rg -nC3 --type=go 'show-policy-docs-link|ShowPolicyDocsLink' cmd internal
printf '\nPolicy-doc-link template/report references:\n'
rg -nC3 'conforma\.dev/docs/policy|policy documentation' cmd internalRepository: conforma/cli
Length of output: 23965
Move show-policy-docs-link flag to individual subcommands or add support in validate policy.
The flag is registered on the parent validate command (line 48), making it available to all subcommands. However, only validate image, validate input, and validate vsa actually read and use this flag. ValidatePolicyCmd never reads it, so ec validate policy --show-policy-docs-link=... becomes a silent no-op for users.
Either:
- Register the flag only on subcommands that use it (image, input, vsa), or
- Add support to consume it in the policy validation command.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/validate/validate.go` at line 48, The flag "show-policy-docs-link" is
registered on the parent validate command (validateCmd) but only used by
specific subcommands (validate image, validate input, validate vsa) while
ValidatePolicyCmd never reads it; either move the flag registration off
validateCmd and register it only on the subcommands that use it (the
image/input/vsa command builders where flags are currently read), or add logic
in the ValidatePolicyCmd handler to read and honor the "show-policy-docs-link"
persistent flag (use the same flag key) so ec validate policy
--show-policy-docs-link behaves consistently. Ensure you update the flag
registration location(s) and the code path that reads the flag (the existing
readers used by image/input/vsa or the ValidatePolicyCmd) so there are no unused
persistent flags and behavior is consistent.
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I know we generally prefer not to change existing behavior when adding a new flag, but for demos it's better if the default is false, otherwise every example in a demo needs to include the flag. Wdyt? Also:
|
I thought changing the CRD definition for just a docs info from the CLI would be overkill, so I settled for the flag idea. But my latest commit changes the behaviour from using a logic that checks if the user is passing snapshot, then they'd be shown the docs link. But you raised a good point in the standup, so I'm thinking of switching back to the flag idea, and then there's the CRD-based plan. What are your thoughts?
Sorry yes, updated now. I will update the PR title and description once we settle on the approach. |
Give users control over policy documentation links in validation reports. Defaults to false to keep demo output clean—production CI can opt in with --show-policy-docs-link=true when they want the help link. Why default to false? Your colleague nailed it: "for demos it's better if the default is false, otherwise every example needs to include the flag." Nobody wants documentation URLs cluttering their quick validation examples. Implementation: - Added persistent flag on parent validate command (all subcommands inherit) - Flag controls display of https://conforma.dev/docs/policy/ link - Link only appears when violations/warnings exist - Updated tests to properly initialize flags - Updated snapshots for new default behavior Usage: # Default - clean output for demos ec validate image --image <img> --policy <policy> # Opt-in for production/CI ec validate image --image <img> --policy <policy> --show-policy-docs-link=true resolves: EC-1603 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoAdd --show-policy-docs-link flag to control policy documentation link visibility
WalkthroughsDescription• Add --show-policy-docs-link persistent flag to control policy documentation link display • Default value changed to false for cleaner demo output • Flag inherited by all validate subcommands (image, input, vsa) • Updated report structures and templates to respect the new flag setting • Updated tests and documentation to reflect new parameter Diagramflowchart LR
A["validate command"] -->|"adds persistent flag"| B["--show-policy-docs-link"]
B -->|"inherited by"| C["validate image"]
B -->|"inherited by"| D["validate input"]
B -->|"inherited by"| E["validate vsa"]
C -->|"passes to"| F["ReportData"]
D -->|"passes to"| F
E -->|"passes to"| F
F -->|"controls display in"| G["text_report.tmpl"]
G -->|"shows/hides"| H["policy documentation link"]
File Changes1. cmd/validate/validate.go
|
Code Review by Qodo
1. Wrong flag default
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/applicationsnapshot/report.go (1)
132-165: Replace the bool train with a small options struct.
NewReport(..., showSuccesses, showWarnings, showPolicyDocsLink, expansion)is already yielding opaque call sites likeNewReport(..., true, true, true, nil)ininternal/applicationsnapshot/report_test.go. That is easy to misorder and gets worse each time a report toggle is added. Worth switching to aReportOptionsstruct here, and applying the same shape tointernal/input.NewReportwhile this flag is still new.As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/applicationsnapshot/report.go` around lines 132 - 165, Introduce a small ReportOptions struct (e.g., type ReportOptions struct { ShowSuccesses, ShowWarnings, ShowPolicyDocsLink bool; Expansion *ExpansionInfo }) and replace the boolean flags + expansion parameter in NewReport with a single opts ReportOptions parameter; update NewReport to read opts.ShowSuccesses, opts.ShowWarnings, opts.ShowPolicyDocsLink and opts.Expansion when building the Report, and adjust all call sites (notably internal/input.NewReport and internal/applicationsnapshot/report_test.go) to pass a ReportOptions value instead of positional booleans so callers become explicit and order-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/modules/ROOT/pages/ec_validate_input.adoc`:
- Line 79: The --show-policy-docs-link flag currently has a default of false;
change its registration in the validate command (the flag registration that
defines "show-policy-docs-link" in validate.go) to use Default: true (or set the
Bool flag default to true) and update the documentation entry that shows the
default value (the line displaying "--show-policy-docs-link:: ... (Default:
false)" in the ec_validate_input.adoc page) to reflect "(Default: true)". Ensure
the flag variable/name remains unchanged and only the default and doc text are
updated so behavior and help output match.
---
Nitpick comments:
In `@internal/applicationsnapshot/report.go`:
- Around line 132-165: Introduce a small ReportOptions struct (e.g., type
ReportOptions struct { ShowSuccesses, ShowWarnings, ShowPolicyDocsLink bool;
Expansion *ExpansionInfo }) and replace the boolean flags + expansion parameter
in NewReport with a single opts ReportOptions parameter; update NewReport to
read opts.ShowSuccesses, opts.ShowWarnings, opts.ShowPolicyDocsLink and
opts.Expansion when building the Report, and adjust all call sites (notably
internal/input.NewReport and internal/applicationsnapshot/report_test.go) to
pass a ReportOptions value instead of positional booleans so callers become
explicit and order-safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c039650f-59aa-499b-b08b-f08c6355cac4
⛔ Files ignored due to path filters (9)
features/__snapshots__/conftest_test.snapis excluded by!**/*.snapfeatures/__snapshots__/initialize.snapis excluded by!**/*.snapfeatures/__snapshots__/inspect_policy.snapis excluded by!**/*.snapfeatures/__snapshots__/ta_task_validate_image.snapis excluded by!**/*.snapfeatures/__snapshots__/task_validate_image.snapis excluded by!**/*.snapfeatures/__snapshots__/track_bundle.snapis excluded by!**/*.snapfeatures/__snapshots__/validate_image.snapis excluded by!**/*.snapfeatures/__snapshots__/validate_input.snapis excluded by!**/*.snapfeatures/__snapshots__/vsa.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
cmd/validate/image.gocmd/validate/input.gocmd/validate/validate.gocmd/validate/vsa.gocmd/validate/vsa_test.godocs/modules/ROOT/pages/ec_validate.adocdocs/modules/ROOT/pages/ec_validate_image.adocdocs/modules/ROOT/pages/ec_validate_input.adocdocs/modules/ROOT/pages/ec_validate_policy.adocdocs/modules/ROOT/pages/ec_validate_vsa.adocinternal/applicationsnapshot/report.gointernal/applicationsnapshot/report_test.gointernal/applicationsnapshot/templates/text_report.tmplinternal/input/report.gointernal/input/report_test.gointernal/input/templates/text_report.tmplinternal/validate/report.go
✅ Files skipped from review due to trivial changes (3)
- docs/modules/ROOT/pages/ec_validate_policy.adoc
- docs/modules/ROOT/pages/ec_validate_image.adoc
- cmd/validate/vsa_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/modules/ROOT/pages/ec_validate.adoc
- cmd/validate/validate.go
- internal/applicationsnapshot/templates/text_report.tmpl
- docs/modules/ROOT/pages/ec_validate_vsa.adoc
- internal/input/report_test.go
- cmd/validate/vsa.go
| --retry-jitter:: randomness factor for backoff calculation (0.0-1.0) (Default: 0.1) | ||
| --retry-max-retry:: maximum number of retry attempts (Default: 3) | ||
| --retry-max-wait:: maximum wait time between retries (Default: 3s) | ||
| --show-policy-docs-link:: Show link to policy documentation in output when there are violations or warnings (Default: false) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --type=go 'show-policy-docs-link' cmd/validate
printf '\n'
rg -n -C1 --type=adoc 'show-policy-docs-link' docs/modules/ROOT/pagesRepository: conforma/cli
Length of output: 2464
Update flag default to true for backward compatibility.
The --show-policy-docs-link flag defaults to false in both code (cmd/validate/validate.go:48) and documentation (line 79), but backward compatibility requires it to default to true. This flips the behavior from opt-out to opt-in, breaking existing workflows. Update the flag registration and documentation to use Default: true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/modules/ROOT/pages/ec_validate_input.adoc` at line 79, The
--show-policy-docs-link flag currently has a default of false; change its
registration in the validate command (the flag registration that defines
"show-policy-docs-link" in validate.go) to use Default: true (or set the Bool
flag default to true) and update the documentation entry that shows the default
value (the line displaying "--show-policy-docs-link:: ... (Default: false)" in
the ec_validate_input.adoc page) to reflect "(Default: true)". Ensure the flag
variable/name remains unchanged and only the default and doc text are updated so
behavior and help output match.
| validateCmd.PersistentFlags().Bool("show-successes", false, "") | ||
| validateCmd.PersistentFlags().Bool("show-warnings", true, "") | ||
| validateCmd.PersistentFlags().Bool("show-policy-docs-link", false, "Show link to policy documentation in output when there are violations or warnings") | ||
| return validateCmd |
There was a problem hiding this comment.
1. Wrong flag default 🐞 Bug ✓ Correctness
The new persistent flag --show-policy-docs-link is registered with default=false, so the documentation link will be hidden by default and validate output changes from prior behavior (and from the PR description).
Agent Prompt
### Issue description
`--show-policy-docs-link` is intended to default to `true` (backward compatible), but it is currently registered with a default of `false`. Because the text report templates now require `ShowPolicyDocsLink` to be true to render the docs link, the docs link is suppressed by default.
### Issue Context
This impacts validate subcommands inheriting the persistent flag (`validate image`, `validate input`, `validate vsa` fallback reports).
### Fix Focus Areas
- cmd/validate/validate.go[46-49]
- Change the default value to `true`.
- docs/modules/ROOT/pages/ec_validate.adoc[5-10]
- Update displayed default to `true`.
- docs/modules/ROOT/pages/ec_validate_image.adoc[172-177]
- docs/modules/ROOT/pages/ec_validate_input.adoc[76-81]
- docs/modules/ROOT/pages/ec_validate_policy.adoc[37-42]
- docs/modules/ROOT/pages/ec_validate_vsa.adoc[60-65]
- Update displayed defaults consistently.
### Notes
After changing the default, consider adding/adjusting tests that validate the docs link appears by default when warnings/violations exist.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
I updated the PR description to reflect the current beahviour. Recheck :whip:
Summary
Adds
--show-policy-docs-linkflag to control policy documentation links in validation output. Defaults tofalsefor clean demo output.Why
Automatic link display cluttered demos. As noted in review: "for demos it's better if the default is false, otherwise every example needs to include the flag."
Changes
validatecommand (inherited by all subcommands)image.go,input.go,vsa.goUsage
Resolves
EC-1603