feat: execute all steps and collect errors at end of chain#1640
feat: execute all steps and collect errors at end of chain#1640jescalada wants to merge 4 commits into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1640 +/- ##
==========================================
+ Coverage 85.87% 85.92% +0.04%
==========================================
Files 84 84
Lines 8109 8161 +52
Branches 1376 1395 +19
==========================================
+ Hits 6964 7012 +48
- Misses 1117 1121 +4
Partials 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coopernetes
left a comment
coopernetes
left a comment
There was a problem hiding this comment.
Overall LGTM, just a few notes/suggestions.
| * single push, instead of stopping at the first failed check. Security and infrastructure | ||
| * failures still stop the chain immediately. | ||
| */ | ||
| collectAllChainErrors?: boolean; |
There was a problem hiding this comment.
Does this need to be configurable? Seems like a sensible new default. Since it only affects the response behaviour back to git clients, it shouldn't be considered a breaking change and needing a toggle.
| const collectibleSteps = new Set<Processor['exec']>([ | ||
| proc.push.checkMessages, | ||
| proc.push.checkAuthorEmails, | ||
| proc.push.checkHiddenCommits, |
There was a problem hiding this comment.
Given that this step resolved an exploit, I would recommend making this one a "fail out early" step. The rest of the checks are sound and should run when a valid push with a set of walkable (traceable from base HEAD/main) refs is sent.
| * such as repository authorisation, user push permission, or later steps | ||
| * depending on their output (pullRemote, writePack, getDiff) | ||
| */ | ||
| const collectibleSteps = new Set<Processor['exec']>([ |
There was a problem hiding this comment.
Worth adding a class/object property like private readonly boolean isCollectible directly on the Processor type? Then each step can self-declare it's behaviour and this list doesn't need to remain statically defined (and needing updates when new checks are introduced).
2 participants
Description
This PR implements one of fogwall's quality-of-life features: collecting all errors in the chain and reporting to the user, so they don't have to make multiple pushes and fix one-by-one. This still fails mid-chain if non-validator steps (
parsePush,pullRemote,writePack, etc.) error out.collectibleStepsare defined inchain.ts(steps that can be collected and reported later, as opposed to immediately failing).Related Issue
Resolves #
Checklist
General
Documentation
Configuration
config.schema.json) was modified:npm run generate-config-types)npm run gen-schema-doc)Tests
npm test)npm run lintandnpm run format:check)npm run check-types)