-
Notifications
You must be signed in to change notification settings - Fork 160
linter: Added new fetch/templating linter #2278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… git checkouts use proper version templating and reproducibility safeguards Signed-off-by: Azimjon Ulmasov <azimjon.ulmasov@chainguard.dev>
58251c5 to
a50f403
Compare
egibs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me at first glance. My main concerns are around maintainability and fragility in parsing the pipelines.
I think it would be appropriate to get a second review from the OS team as well.
|
|
||
| /* | ||
| Rules: | ||
| - Rule A: At least one source must use templates (e.g., ${{package.version}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this warn if there are multiple sources and only one source uses templating in addition to failing if none of the sources use templating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's tricky. I was basing this off of One challenge is that a package can have multiple fetch / origins, not all of which would be parameterized on the package version or other template variable, so it would need to be something like "flag if none of the fetch locations include a variable comment
|
|
||
| return &packagePatterns{ | ||
| exactVersionURL: regexp.MustCompile( | ||
| `(?:^|/)` + escName + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these patterns be defined as variables for easier reference? Would it make sense to also fuzz/test each to ensure sure they do or do not match what we'd expect?
| - Rule A: At least one source must use templates (e.g., ${{package.version}}) | ||
| - Rule B: Fetch URLs with hardcoded package versions should use version templates | ||
| - Rule C: Git tags with hardcoded versions should use version templates | ||
| - Rule D: Git branches/refs require expected-commit for reproducibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we should require expected-sha256 or an equivalent for fetches (unless this already exists somewhere and I'm misremembering).
| return "" | ||
| } | ||
|
|
||
| var processPipelines func(*yaml.Node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to have findValue, getString, and processPipelines exist as separate functions for maintainability and to improve future testing?
justinvreeland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the results of this run on all the files we have now.
I have some concerns about files that should be fixed up that seem like they'd not work with this. gn.yaml and jenkins-2.yaml in wolfi should ideally use git checkout. fluent-plugin-newrelic.yaml will fail this I think but that might be a poorly formed package in general. I think the matching might be a bit too strict. We have ingress-nginx-controller-1.13.yaml and opentelemetry-cpp-contrib.yaml which define their own variables to old the versions in that should pass this ideally but i think will fail; though I guess they're already opting out of a number of git related pipelines.
It'd be nice to get a list of all the current warnings and see if they can be fixed or pre-emptively add exceptions so this can be turned on as more than a warning.
I'm not sure about the placement of the linter here. IIRC melange linters are mostly linting packages not the melange files themselves. I thought there was work or plans to begin unifying them but looking at this I'm pretty sure that the build configuration you're being passed here as config is the fully processed build configuration struct not the raw yaml from the file.
| // Runs all validation rules and returns formatted errors | ||
| func (v *validator) validateAll(sources sourceData) error { | ||
| // Rule A: Template requirement | ||
| if err := v.validateRuleA(sources); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is A different than the other issues?
|
You do call the same function to parse the configuration but it does look like it should be performing variable substituion: Line 1529 in 82f1210
|
Missed that cfg.Root preserves the original tree. |
This PR introduces a new linter,
fetch/templating, which validates that packagefetchsources andgit checkoutsuse proper version templating and reproducibility safeguards. It is designed to prevent version drift, silent breakage and non-reproducible builds, including the class of issues that previously allowed outdated sudo sources to go unnoticed.It operates directly on the raw YAML (pre-template expansion), so it sees the actual
uri,tagandrefstrings as written in the recipe. It enforces a small set of rules:Rule
A: At least one source must use templates (e.g., ${{package.version}})Rule
B: Fetch URLs with hardcoded package versions should use version templatesRule
C: Git tags with hardcoded versions should use version templatesRule
D: Git branches/refs require expected-commit for reproducibilityExamples: