feat(#1313): accept positional args for validate input#3382
feat(#1313): accept positional args for validate input#3382fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
Change ec validate input to accept input file paths as positional arguments instead of requiring the --file flag. This enables natural shell glob usage like ec validate input *.yaml. The --file flag is deprecated but still accepted for backward compatibility. If both positional arguments and --file are provided, the values are combined. A deprecation warning is logged to stderr when --file is used, avoiding corruption of structured output formats like JSON. Changes: - cmd/validate/input.go: add cobra.ArbitraryArgs, merge positional args into filePaths, replace MarkFlagRequired with MarkHidden, add manual validation for at least one input file, update usage/examples - cmd/validate/input_test.go: add tests for positional args, multiple positional args, mixed positional and flag usage, update no-file-provided test for new error message Closes #1313
|
🤖 Finished Review · ✅ Success · Started 9:02 AM UTC · Completed 9:13 AM UTC |
Review — commentSummary: This PR changes Findings1.
|
| ec validate input /path/to/file.yaml --policy github.com/user/repo | ||
|
|
||
| `), | ||
| Args: cobra.ArbitraryArgs, |
There was a problem hiding this comment.
No test covers positional args that are empty strings, non-existent files, or flag-like strings (e.g. --polcy). These are untested boundary conditions between argument parsing and file access.
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 2:33 PM UTC · Completed 2:55 PM UTC |
- Move file-path merge and validation from RunE to PreRunE for earlier error reporting before expensive policy initialization - Add deduplication of file paths when both --file flag and positional arguments provide the same file - Update all 7 acceptance test scenarios to use positional arguments instead of --file flag, preventing CI failures from deprecation warning on stderr - Regenerate auto-generated documentation (ec_validate_input.adoc) - Remove unnecessary SetTestRekorPublicKey from no-file-provided test Addresses review feedback on #3382
🔧 Fix agent — iteration 1 (human-triggered)Addressed 4 of 5 review findings. Fixed the high-severity acceptance test breakage by converting all scenarios to positional args. Regenerated auto-generated docs. Moved validation to PreRunE for earlier error reporting. Added file path deduplication. Disagreed with converting existing unit tests (out of scope enhancement). Fixed (4):
Disagreed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Review · ❌ Terminated · Started 2:59 PM UTC · Ended 3:13 PM UTC |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
🤖 Finished Review · ✅ Success · Started 2:59 PM UTC · Completed 3:13 PM UTC |
|
A few things from the previous review round are still open. The fix agent addressed the biggest items (PreRunE timing, dedup, acceptance tests, docs regen), but these were left untouched:
|
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 12:12 PM UTC · Completed 12:35 PM UTC |
- Replace cobra.ArbitraryArgs with custom validator rejecting flag-like args - Remove log.Warn deprecation warning to match image.go silent pattern - Use errors.Join for empty-files check (error accumulation consistency) - Add deprecated --file multi-file patterns to examples/docs - Fix Use field ordering to show files before flags - Consolidate 3 positional arg tests into table-driven test with require - Add dedup test case and flag-like arg rejection test - Regenerate ec_validate_input.adoc via go generate Addresses review feedback on #3382
|
🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:42 PM UTC |
|
/fs-fix address all the unresolved inline comments |
|
🤖 Finished Fix · ✅ Success · Started 3:47 PM UTC · Completed 4:05 PM UTC |
- Add early return in PreRunE when no input files provided, avoiding unnecessary policy initialization and potential network I/O - Add test case for single-dash flag-like positional args (-x) - Document early-return contract in NoFileProvided test comment Addresses review feedback on #3382
|
🤖 Finished Review · ✅ Success · Started 4:06 PM UTC · Completed 4:14 PM UTC |
| Enable color when using text output even when the current terminal does not support it`)) | ||
|
|
||
| if err := cmd.MarkFlagRequired("file"); err != nil { | ||
| if err := cmd.Flags().MarkHidden("file"); err != nil { |
There was a problem hiding this comment.
[medium] correctness
MarkHidden is used instead of MarkDeprecated, so no deprecation warning is emitted to stderr when --file is used. The PR description claims a deprecation warning is logged, but MarkHidden only hides the flag from help — it does not produce any runtime warning. Cobra's MarkDeprecated function both hides the flag and emits a warning, which is the stated behavior.
Suggested fix: Replace cmd.Flags().MarkHidden("file") with cmd.Flags().MarkDeprecated("file", "use positional arguments instead"). MarkDeprecated calls MarkHidden internally, so no other changes are needed.
| 'git::'. For the policy configuration files you can use json instead of yaml if you prefer. | ||
|
|
||
| ec validate input --file /path/to/file.json --policy github.com/user/repo//default?ref=main | ||
| ec validate input /path/to/file.json --policy github.com/user/repo//default?ref=main |
There was a problem hiding this comment.
[low] correctness
The custom Args validator rejects any positional argument starting with '-', which prevents valid filenames like '-report.yaml' from being used even after the '--' end-of-flags marker. Additionally, this validator is redundant — Cobra already rejects unknown flags during its own flag parsing phase before Args is called. The two test cases in Test_ValidateInputCmd_InvalidPositionalArgs exercise Cobra's built-in error handling, not this custom validator.
Suggested fix: Consider removing the custom Args function entirely (Cobra's default allows arbitrary args), or at minimum accept dash-prefixed args after validating they aren't known flag patterns.
| version: "0.1" | ||
| """ | ||
| When ec command is run with "validate input --file pipeline_definition.yaml --policy git::https://${GITHOST}/git/happy-day-config.git --output json" | ||
| When ec command is run with "validate input pipeline_definition.yaml --policy git::https://${GITHOST}/git/happy-day-config.git --output json" |
There was a problem hiding this comment.
[low] test-coverage
All seven acceptance test scenarios were migrated from --file to positional args. No acceptance-level test remains for the deprecated --file flag's backward compatibility. Unit tests still cover --file, but the acceptance test gap means end-to-end backward compatibility is not validated.
Suggested fix: Keep at least one acceptance scenario using the --file flag to ensure end-to-end backward compatibility is tested.
Change ec validate input to accept input file paths as positional arguments instead of requiring the --file flag. This enables natural shell glob usage like ec validate input *.yaml.
The --file flag is deprecated but still accepted for backward compatibility. If both positional arguments and --file are provided, the values are combined. A deprecation warning is logged to stderr when --file is used, avoiding corruption of structured output formats like JSON.
Changes:
positional args into filePaths, replace MarkFlagRequired
with MarkHidden, add manual validation for at least one
input file, update usage/examples
multiple positional args, mixed positional and flag usage,
update no-file-provided test for new error message
Closes #1313
Post-script verification
agent/1313-positional-args-input)d134e495b18cf8493cfea8791e310b58f0320797..HEAD)