Skip to content
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

Check images for an empty image string #385

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

OchiengEd
Copy link
Contributor

Update the getImagesFromContent function to return all images, including empty ones.

In addition, when evaluating images received in certifyImages() function, skip evaluating registries for any empty images

These changes also have a performance improvement. Unlike the previous implementation of the function that evaluated each line of the generated helm templates for images.

chart-verifier/ (empty-image✗) $ go test -v  -run=^$ -bench=^BenchmarkGetImagesFromContent  github.com/redhat-certification/chart-verifier/internal/chartverifier/checks
goos: darwin
goarch: amd64
pkg: github.com/redhat-certification/chart-verifier/internal/chartverifier/checks
cpu: Intel(R) Core(TM) i7-7700HQ CPU @ 2.80GHz
BenchmarkGetImagesFromContent
BenchmarkGetImagesFromContent-8      	    2780	    429554 ns/op
BenchmarkGetImagesFromContentAlt
BenchmarkGetImagesFromContentAlt-8   	   18494	     64014 ns/op
PASS
ok  	github.com/redhat-certification/chart-verifier/internal/chartverifier/checks	4.940s
chart-verifier/ (empty-image✗) $

Closes #347

@OchiengEd OchiengEd force-pushed the empty-image branch 2 times, most recently from 6f07ef1 to 00c6a15 Compare July 25, 2023 20:02
Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions ^

@OchiengEd OchiengEd force-pushed the empty-image branch 3 times, most recently from 4e4e8d8 to 8dcdf16 Compare July 31, 2023 18:58
Update the getImagesFromContent function to return all images, including empty ones.

In addition, when evaluating images received in certifyImages() function, skip evaluating registries for any empty images

Signed-off-by: Edmund Ochieng <ochienged@gmail.com>
Signed-off-by: Edmund Ochieng <ochienged@gmail.com>
Signed-off-by: Edmund Ochieng <ochienged@gmail.com>
Copy link
Contributor

@komish komish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks @OchiengEd

@komish komish merged commit 8c331d0 into redhat-certification:main Aug 1, 2023
@OchiengEd OchiengEd deleted the empty-image branch August 1, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charts should fail cleanly when an deployment resource's image value is set to an empty string
2 participants