feat(find-images): 4509 include archives in find image#4551
feat(find-images): 4509 include archives in find image#4551chaospuppy wants to merge 44 commits intozarf-dev:mainfrom
Conversation
✅ Deploy Preview for zarf-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@AustinAbro321 when you have a moment, please take a look. I am still going to update a few unit tests. |
aad61df to
6d872f4
Compare
b03b517 to
f35514c
Compare
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
AustinAbro321
left a comment
There was a problem hiding this comment.
Good start! Thanks for taking this on
| // It returns a list of images found in the extracted OCI layout. | ||
| func FindImagesInArchive(ctx context.Context, imageArchive, destDir string) ([]string, error) { | ||
| // Create a temporary directory for extraction | ||
| tmpdir, err := utils.MakeTempDir("") |
There was a problem hiding this comment.
The extraction logic in this block is mostly duplicated. Lets make a common function to de-duplicate it
There was a problem hiding this comment.
Made a few changes with this in mind
| // SkipVersionCheck skips version requirement validation | ||
| SkipVersionCheck bool | ||
| // SkipImageArchivesImages ignores schema validation errors when imageArchives does not include an images list | ||
| SkipEmptyImageArchivesImages bool |
There was a problem hiding this comment.
I'd prefer a strategy where we create a function load.PackageDefinitionNoValidate() then also make a public function load.Validate(). Keep the load.PackageDefinition function this way validation happens by default.
I believe a public validate function could be generally useful for SDK users who might be creating packages without zarf.yaml files, additionally it'll leave more room in the future for more generic solutions if we need to skip validation for a similar reason
There was a problem hiding this comment.
I'm happy to make that split still, but our failure was while validating the file itself here, rather than during the validation of v1alpha1.ZarfPackage. Therefore, it wasn't useful for us to delay validation after adding a placeholder image as we previously discussed to the v1alpha1.ZarfPackage.
I could split them and then add some work to change the zarf.ymal on disk to add an empty array there, but that didn't seem like desirable behavior.
There was a problem hiding this comment.
Alternatively, I could also split out a Validate and ValidateWithoutSchemaValidation (not literally that name, but something like that) and use the later
| return nil, fmt.Errorf("failed to unpack image archive %s: %w", archive.Path, err) | ||
| } | ||
| for _, image := range archiveImages { | ||
| matchedImages[image] = true |
There was a problem hiding this comment.
I think matched images isn't the right construct. Maybe a new field should be added such as archiveImages. One of the goals of find-images is to be able to copy and paste the output into your zarf.yaml. Currently you'd see this output, which would result in trying to pull the image from the internet if you copied and pasted it
2026-01-27 16:38:42 INF looking up cosign artifacts for discovered images count=3
components:
- name: kiwix-serve
images:
- docker.io/library/kiwix-data:local
- ghcr.io/kiwix/kiwix-serve:3.5.0-2
- kiwix-data:localThere was a problem hiding this comment.
I see, good point, I missed the desire to pull images from the image archive on disk after zarf.yaml has been updated with the output of zarf dev find-images. I'll fix that up!
There was a problem hiding this comment.
@AustinAbro321 to clarify, are you hoping to see images elements expressed in a such as way as to direct Zarf to pull them from the archive, rather than from the internet?
There was a problem hiding this comment.
Ah I missed your comment before asking, I think I see the answer in #4577
There was a problem hiding this comment.
Just went back and clarified #4577 a bit, but just to write here as well, we should no longer include images because they're in an image archive. If they're in both a manifest we should check if the image archive contains that image, then in the output to the user structure it so it's in the image archive, and the user can simply copy and paste.
08c339e to
9534ea6
Compare
|
Hello @chaospuppy, I reazlied that the behavior I proposed in #4509, didn't really fit the If you do adjust this PR for #4577, don't worry about validation, assume the given zarf.yaml file is valid |
|
@AustinAbro321 Are you thinking about handling validation differently in the near future? Trying to get an understanding for which changes I should back out. I am also happy to keep the validation changes in, including making Validation public and creating PackageDefinitionWithoutValidation. |
|
@chaospuppy Very possibly handling validation in the near future yes. I suggest backing out of the validation changes in this PR |
…eholder images list, add skipimagearchivesimages definition option Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…efinitionOption for skipping imagearchives images schema findings Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…es output Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…dev#4554) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…-organization group (zarf-dev#4553) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…ev#4552) Signed-off-by: Jason Washburn <jason.washburn@gmail.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com> Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
|
|
||
| type ImageArchivesScan struct { | ||
| ComponentName string | ||
| ImageArchives []ImageArchive |
There was a problem hiding this comment.
I considered using v1alpha1.ZarfComponent.ImageArchive here, but thought perhaps there could be some divergence in the future. Let me know if you'd prefer the re-use of v1alpha1.ZarfComponent.ImageArchive
There was a problem hiding this comment.
I think they are coupled in that if image archives were to change, we'd also want to change this block since ultimately we'd want to use the information here to update / print a package definition
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
AustinAbro321
left a comment
There was a problem hiding this comment.
Good progress. When running this locally on examples/kiwix I get
components:
- name: kiwix-serve
images:
- ghcr.io/kiwix/kiwix-serve:3.5.0-2
- kiwix-data:local
# Archive images - kiwix-serve
- name: kiwix-serve
imageArchives:
- path: kiwix-data.tar
images: []Whereas I'd expect
- name: kiwix-serve
images:
- ghcr.io/kiwix/kiwix-serve:3.5.0-2
imageArchives:
- path: kiwix-data.tar
images:
- kiwix-data:localThe kiwix-data:local is in the wrong spot, and they are printed as different components.
|
|
||
| type ImageArchivesScan struct { | ||
| ComponentName string | ||
| ImageArchives []ImageArchive |
There was a problem hiding this comment.
I think they are coupled in that if image archives were to change, we'd also want to change this block since ultimately we'd want to use the information here to update / print a package definition
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
|
@AustinAbro321 I've resolved the duplicate components issue, thanks for spotting that. To your point about |
|
So a fun thing about containers is that if the registry is not specified then it's always assumed to be You should be able to use the |
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
|
@AustinAbro321 updated to return qualified names for scan |
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
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.
Good progress, some more requests as I go through
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
…ind-image Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
987f2e9 to
3327eae
Compare
Signed-off-by: Tim Seagren <timseagren@defenseunicorns.com>
AustinAbro321
left a comment
There was a problem hiding this comment.
Requesting an architecture change. Appreciate the iterations
| // FilterImagesFoundInArchives recieves imageScans and discovers if any of its elements are duplicated by | ||
| // images provided in imageArchives parsed out of the package located at packagePath. If duplciates are found, they | ||
| // are removed from imageScans and added to archiveImageScans. | ||
| func FilterImagesFoundInArchives(ctx context.Context, packagePath string, imageScans []ComponentImageScan) (_ []ImageArchivesScan, _ []ComponentImageScan, err error) { |
There was a problem hiding this comment.
I'm thinking more about the architecture we're going with here. I'd like to see this function private and another abstract function introduced FindDefinitionImages. This function would return a struct like this, and would do the logic of filtering the images.
type DefinitionImageResult struct {
ComponentImageScan
ImageArchives []v1alpha1.ImageArchive
} This should solve a few problems:
- dev.go and UpdateImages will be simplified since all data will already be in one struct
- when --why is used, we can just call the original Packager.FindImages, it's no longer order dependent in dev.go
- We can introduce a private function packager.findImages which accepts a package definition. This way we only have to load the package once.
There was a problem hiding this comment.
I'm a little confused around FindDefinitionImages returning a struct like DefinitionImageResult. I'm thinking FindDefinitionImages would need to return a []DefinitionImageResult to that the rest of the logic in dev.go and UpdateImages can remain relatively unchanged. Is that right?
Aside from that, I'm also not totally certain I understand the situations in which we call packager.FindImages, packer.findImages, and packager.FindDefintionImages. My current idea is this:
FindImagesis only called if--whyis set, indev.goFindDefinitionImagesis called if--whyis not set, indev.gofindImagesis called by bothFindImagesandFindDefinitionImages- Both
FindImagesandFindDefintitionImageshandle loading the package before passing it as an argument tofindImages
Is that right?
There was a problem hiding this comment.
Yes, I misspoke, it would return the struct. Your understanding and flow is correct
Description
This MR resolves #4577 by doing the following:
imageArchiveskeys. Currently this finding is only removed when calling PackageDefinition from FindImagesimageArchives.pathRelated Issue
Fixes #4577
Checklist before merging