fix: add escape hatch for when signing service is down.#793
Conversation
Signed-off-by: Giles Cope <gilescope@gmail.com>
m2ux
left a comment
There was a problem hiding this comment.
PR Review Summary
PR: #793 - fix: add escape hatch for when signing service is down.
Reviewers: Code Review Agent · Test Suite Review Agent · Validation Agent · Strategic Review Agent
Reports: Code Review · Test Suite Review
Date: 2026-02-26
Executive Summary
Clean, well-scoped change that surfaces an existing skip-attestation parameter from sbom-scan-image.yml as an opt-in workflow_dispatch input in release-image.yml. All 6 SBOM scan jobs correctly receive the passthrough, default behavior is preserved, and the PR is better-scoped than its sibling PR #790 targeting main. No blocking findings.
Overall Rating: Approve
Note: This PR was already merged at the time of review. Findings are retrospective.
Code Review Findings
| # | Finding | Severity |
|---|---|---|
| CR-1 | Redundant || false fallback in passthrough expressions |
Low |
| CR-2 | Conventional commit prefix fix: could be feat: for new capability |
Low |
Finding Details
CR-1. Redundant fallback expression (Low)
All 6 passthrough lines use ${{ inputs.skip-attestation || false }}. The || false is redundant because the input is defined with default: false. The simpler ${{ inputs.skip-attestation }} would be equivalent.
Assessment: Defensive coding, consistent with PR #790. No behavioral impact.
Suggestion: Optional cleanup in a future pass if the team prefers minimal expressions.
CR-2. Conventional commit prefix (Low)
The commit uses fix: but this change adds a new operational capability (the escape hatch) rather than correcting broken behavior. feat: or chore: could be more semantically precise.
Assessment: Minor. The fix: prefix is defensible since the inability to release during outages is a deficiency.
Suggestion: Informational only — no action needed on a merged PR.
Test Review Findings
| # | Gap | Severity |
|---|---|---|
| TR-1 | No actionlint or yamllint CI step for workflow files |
Low |
| TR-2 | No automated check that all SBOM scan jobs include skip-attestation |
Low |
Finding Details
TR-1. No workflow YAML validation in CI (Low)
The repository does not have actionlint or yamllint as a CI step for PRs modifying .github/workflows/*.yml. This would catch syntax and schema errors before merge.
Suggestion: Consider adding an actionlint CI job for workflow file changes.
TR-2. No passthrough completeness check (Low)
If a new SBOM scan job is added to release-image.yml in the future, the skip-attestation passthrough could be missed. A simple grep-based CI check could prevent this.
Suggestion: Low priority given the infrequency of changes to this workflow.
Validation Findings
All validation checks passed. No non-passing findings.
| Check | Result |
|---|---|
| YAML syntax | Pass |
| Input definition (boolean, default false) | Pass |
| Passthrough completeness (6/6 jobs) | Pass |
Input compatibility with sbom-scan-image.yml |
Pass |
| Existing behavior preserved | Pass |
Branch Hygiene
No issues found.
| Check | Result |
|---|---|
| Single-purpose commit | Pass |
| No scope leak | Pass — contrast with PR #790 which bundles an audit fix |
| Clean merge | Pass — merged without conflicts |
Action Items
Could Address (Suggested):
- Simplify
${{ inputs.skip-attestation || false }}to${{ inputs.skip-attestation }}in a future cleanup (CR-1) - Add
actionlintCI validation for workflow file changes (TR-1)
Nice to Have (Optional):
- Add automated completeness check for
skip-attestationpassthrough coverage (TR-2) - Use
feat:prefix for commits introducing new capabilities (CR-2)
* change: ETCM-9687 Fixes and documentation for util crates * allow deprecated * change deprecation version * simplify languge * fix clippy warning
* change: ETCM-9687 Fixes and documentation for util crates * allow deprecated * change deprecation version * simplify languge * fix clippy warning Signed-off-by: Mike Clay <mike.clay@shielded.io>
* change: ETCM-9687 Fixes and documentation for util crates * allow deprecated * change deprecation version * simplify languge * fix clippy warning Signed-off-by: Mike Clay <mike.clay@shielded.io>
Similar to #790