feat(lint): add --all-variants flag for lint - #4783
Conversation
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
dbc0447 to
e8ffd82
Compare
… filter our error types from a joined error Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
815d65f to
b771fe5
Compare
AustinAbro321
left a comment
There was a problem hiding this comment.
This should be a nice win for linting, thanks! A few comments
db6ce34 to
a8d6f9d
Compare
…exclusivity Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
| components: | ||
| - name: duplicate | ||
| only: | ||
| flavor: first-flavor |
There was a problem hiding this comment.
Need to test importing a child components with flavors here
| } | ||
|
|
||
| func compatibleComponent(c v1alpha1.ZarfComponent, arch, flavor string) bool { | ||
| func compatibleComponent(c v1alpha1.ZarfComponent, arch, flavor string, allVariants bool) bool { |
There was a problem hiding this comment.
I believe we'll also want all variants of architectures.
There was a problem hiding this comment.
I interpreted this to mean we want allVariants to b evaluated within compatibleComponent for both flavor and arch checks, but I also see how it might be better to leave the function unchanged and instead do something like if !compatibleComponent && !allVariants.
| for _, component := range pkg.Components { | ||
| // ensure component name is unique | ||
| if _, ok := uniqueComponentNames[component.Name]; ok { | ||
| if _, ok := uniqueComponentNames[component.Name]; ok && !opts.SkipComponentNameUniquenessValidation { |
There was a problem hiding this comment.
This is too broad, let's change it so we only allow duplicate component names when the only blocks are not equal
There was a problem hiding this comment.
Since we can have any number of components with duplicate names when this validation option is true, I needed to map component names to all of the only blocks associated with those component names. Let me know if I could simplify further, but I also didn't want to run reflect.DeepEqual unless we enabled SkipComponentNameUniquenessValidation.
…pping name uniqueness checks, update unit tests Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
AustinAbro321
left a comment
There was a problem hiding this comment.
Apologies for the delay in review. A few comments
| validationOpts := internalv1alpha1.ValidateOpts{} | ||
|
|
||
| if allVariants { | ||
| validationOpts.SkipComponentNameUniquenessValidation = true | ||
| } |
There was a problem hiding this comment.
| validationOpts := internalv1alpha1.ValidateOpts{} | |
| if allVariants { | |
| validationOpts.SkipComponentNameUniquenessValidation = true | |
| } | |
| validationOpts := internalv1alpha1.ValidateOpts{ | |
| SkipComponentNameUniquenessValidation: allVariants, | |
| } |
There was a problem hiding this comment.
We'll now need to handle multiple components with the same name being found. Add a test for this as well
| // only check if only block is duplicated if we're skipping name uniqueness checks | ||
| if opts.SkipComponentNameUniquenessValidation { | ||
| duplicateOnly = slices.ContainsFunc(uniqueComponentNames[component.Name], func(o v1alpha1.ZarfComponentOnlyTarget) bool { | ||
| return reflect.DeepEqual(o, component.Only) |
There was a problem hiding this comment.
We generally avoid reflect, I'd do something like this instead
func onlyTargetsEqual(a, b v1alpha1.ZarfComponentOnlyTarget) bool {
return a.LocalOS == b.LocalOS && a.Flavor == b.Flavor &&
a.Cluster.Architecture == b.Cluster.Architecture &&
slices.Equal(a.Cluster.Distros, b.Cluster.Distros)
}| type DefinitionOptions struct { | ||
| Flavor string | ||
| Flavor string | ||
| // All variants will ignore Flavor and will return all components, regardless of flavor |
There was a problem hiding this comment.
| // All variants will ignore Flavor and will return all components, regardless of flavor | |
| // All variants will ignore Flavor & architecture and will return all components. Mutually exclusive with flavor. |
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…e when allVariants, update unit tests to cover new behavior Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
|
@AustinAbro321 sorry for the delay on my end here, I've addressed the feedback you provided above. Setting |
AustinAbro321
left a comment
There was a problem hiding this comment.
I've not done a full review, but asking that this be in line with #5020
| Flavor string | ||
| Flavor string | ||
| // All variants will ignore Flavor & architecture and will return all components. Mutually exclusive with flavor. | ||
| AllVariants bool |
There was a problem hiding this comment.
There is similar work going on in #5020 src/pkg/packager/load/load.go. Instead of a boolean, let's copy over the variant Dimension construct, that way users can selectively add the filters that they want.
Description
This PR adds an
--all-variantsflag to thezarf dev lintcommand that will result in all variants of a zarf package being linted, even if there are multiple flavors present in the package definition. Previously, it was required that users specify a--flavorflag in order to pass validation. This is done to make linting zarf package in CI or in situations where you would otherwise not want to/could not know which flavor you wanted to lint specifically, and wished to validate the entire package.Related Issue
Fixes #4598
Checklist before merging