feat(sbom): add --warnings-non-fatal flag to sbom validate - #331
reyreavman wants to merge 1 commit into
Conversation
Verification
Review focus
Follow-up
|
Verification
Review focus
Follow-up
|
Fral738
left a comment
There was a problem hiding this comment.
The strict-to-lenient default flip is explicitly intentional, but it still changes existing CI acceptance gates from failure to success without any configuration change. --fail-on-warnings is an opt-back, not an opt-in feature flag for the new behavior, so it does not satisfy the repository's rule for breaking user-facing changes. Preserve the strict default and make lenient behavior opt-in, or obtain an explicit compatibility-policy exception before merging this as a fix.
The description also understates the affected population: the pinned checker produces warnings without --check-vcs. For example, werf sbom validate --path=test/e2e/sbom/_fixtures/validate/oss_multiple_vcs_urls.json --ispras-format=oss emits a multiple-VCS-URLs WARNING and exits 0 on this head; the same invocation with --fail-on-warnings exits 1. The new warning-only e2e case also passes no --check-vcs. Please correct both the parenthetical in What and the release rationale: affected pipelines are not limited to those using --check-vcs.
The inline comments cover the missing environment-variable counterpart and the unprotected stdout/stderr contract.
| cmd.Flags().StringArrayVar(&pathFlags, "path", nil, "Path to CycloneDX JSON SBOM file (repeatable)") | ||
| cmd.Flags().StringVar(&isprasFormatFlag, "ispras-format", "", "ISPRAS SBOM format: oss or container") | ||
| cmd.Flags().BoolVar(&checkVCSFlag, "check-vcs", false, "Enable VCS URL validation") | ||
| cmd.Flags().BoolVar(&failOnWarningsFlag, "fail-on-warnings", false, "Treat checker warnings as failures (by default only errors fail validation)") |
There was a problem hiding this comment.
The new flag has no WERF_* environment-variable counterpart, contrary to the CLI convention. With the warning-only fixture, WERF_FAIL_ON_WARNINGS=true werf sbom validate --path=test/e2e/sbom/_fixtures/validate/oss_multiple_vcs_urls.json --ispras-format=oss still exits 0, while adding --fail-on-warnings exits 1.
Use the existing environment-default helper, add the pkg/util import, regenerate the CLI reference, and cover env=true plus an explicit --fail-on-warnings=false override:
cmd.Flags().BoolVar(&failOnWarningsFlag, "fail-on-warnings", util.GetBoolEnvironmentDefaultFalse("WERF_FAIL_ON_WARNINGS"), "Treat checker warnings as failures (default $WERF_FAIL_ON_WARNINGS or false)")| }) | ||
| Expect(out).To(ContainSubstring("OK")) | ||
| if expectedSubstring != "" { | ||
| Expect(out).To(ContainSubstring(expectedSubstring)) |
There was a problem hiding this comment.
The combined-output assertion does not protect the newly documented stderr contract. Changing the warning logger in fileResult.report from Warn() back to Default() leaves all 21 checker unit specs and all 31 validate e2e specs passing, even though WARNING lines are then written to stdout.
Capture stdout and stderr separately for the warning-only fixture and assert the stream contents explicitly; retain the exit-code assertion and use the same logger mutation to verify the test fails:
Expect(stdout).To(ContainSubstring("OK (1 warning(s))"))
Expect(stdout).NotTo(ContainSubstring("WARNING:"))
Expect(stderr).To(ContainSubstring("WARNING:"))sbom validate now reports checker findings split into errors and
warnings, printing both counts in the summary ("N error(s), M
warning(s)"), and warnings are written to stderr instead of stdout.
By default any error or warning still fails validation with exit 1, as
before — the strict default is unchanged. The new --warnings-non-fatal
flag (env WERF_WARNINGS_NON_FATAL) keeps warnings informational so only
errors set a non-zero exit code. This lets a pipeline opt into lenient
acceptance ahead of the ISPRAS checker image bump, which starts emitting
purl-case warnings on otherwise conforming SBOMs.
Signed-off-by: Radmir Khurum <radmir.khurum@flant.com>
b850186 to
a84ecde
Compare
Summary
werf sbom validatenow separates sbom-checker findings into errors and warnings, shows both counts in the summary, and adds an opt-in--warnings-non-fatalflag that keeps warnings from failing the run. The strict default is unchanged: by default any error or warning still exits 1, exactly as before — so no existing pipeline changes behavior without opting in.What
Result: N passed, M failed; X error(s), Y warning(s).OK,OK (M warning(s)), orFAILED; warning lines are written to stderr, error lines and the status header to stdout.--warnings-non-fatal(defaultfalse, envWERF_WARNINGS_NON_FATAL): when set, files whose only findings are warnings pass (exit 0); errors still fail.FAILEDand the run exits 1, as on the current release.validation failed for <file>:followed by the finding lines) is unchanged.Why
parseResultappended warnings to the same list as errors, so the error/warning distinction the checker already makes (--errors 0) was collapsed in the summary and there was no way to accept warnings without also accepting errors. The upcoming ISPRAS checker image bump starts emitting purl-case warnings on otherwise conforming production SBOMs; a pipeline that wants to keep shipping through those warnings can now pass--warnings-non-fatalinstead of losing error gating entirely.The default was deliberately kept strict rather than flipped to lenient: flipping it would turn an existing red CI gate green with no config change, which the repository's compatibility policy disallows without an explicit exception. Leniency is therefore opt-in. A boolean
--warnings-non-fatalwas chosen over--strictness=errors|allbecause the checker emits exactly two finding classes; the enum can be added later if a third appears.