Migrated artifact registry into new validation framework #117
Conversation
jdob
commented
Jan 18, 2024
jdob
commented
- Migrated the isEmbeddedArtifactRegistry check into the combustion file for the registry work since it's primarily used by that component
pkg/image/validation/registry.go
Outdated
| registryComponent = "Artifact Registry" | ||
| ) | ||
|
|
||
| func validateEmbeddedArtefactRegistry(ctx *image.Context) []FailedValidation { |
There was a problem hiding this comment.
This is a nit but Artifact is only spelled with an "e" once in the whole file.
There was a problem hiding this comment.
I know, I was trying to keep consistent with the other places in the code that use Artefact, forgetting it was part of the definition. I'll revert these for consistency within the file.
| "github.com/suse-edge/edge-image-builder/pkg/image" | ||
| ) | ||
|
|
||
| func TestValidateEmbeddedArtefactRegistry(t *testing.T) { |
There was a problem hiding this comment.
Same comment as before, shows up here and on line 65, but only for 2/17 instances.
| "The 'name' field is required for each entry in 'charts'.", | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Another potential nit, I think this coverage is fine, but I think I'd also like to see individual failure cases.
There was a problem hiding this comment.
The individual failure cases are covered in the respective tests for validating container images and helm charts. This test simply shows that both of those are called from this runner function.
| "The 'repoURL' field must begin with either 'http://' or 'https://'.", | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Same nit suggestion regarding individual failure cases (which is probably not necessary to be honest). Though I will say that I really like the way you concatenate the error messages, very cool.
pkg/image/validation/registry.go
Outdated
| if combustion.IsEmbeddedArtifactRegistryEmpty(def.EmbeddedArtifactRegistry) { | ||
| return failures | ||
| } |
There was a problem hiding this comment.
Can we drop this check? Relying on the combustion package for the validation itself seems weird and trying to iterate over nil slices should not lead to errors below.
There was a problem hiding this comment.
I didn't love this dependency either; it was mostly because the check is used at both runtime and validation time. But since this is the only instance where that happens (so far) and like you said it doesn't affect the behavior, I agree with dropping it.
