-
Notifications
You must be signed in to change notification settings - Fork 41
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
Store entire bundle, generate relocation map #65
Conversation
if len(b.InvocationImages) == 0 { | ||
return nil, fmt.Errorf("unknown invocation image: %q", d.Digest) | ||
} | ||
relocationMap[b.InvocationImages[0].Image] = refFamiliar |
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.
Here we still only handle the first invocation image from a bundle - at some future point we should handle multiple invocation images.
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.
Agreed. Is the current restriction captured in an issue?
7b5c27b
to
45fdf5a
Compare
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.
Manually tested the latest rebase, LGTM.
Thanks a lot, @silvin-lubecki!
PTAL @glyn 🙏 |
51fc7a3
to
541c483
Compare
} | ||
// Fixup images | ||
for name, original := range b.Images { | ||
if original.BaseImage, err = fixupImage(ctx, original.BaseImage, cfg, events, cfg.componentImagePlatformFilter); err != nil { | ||
return err | ||
if err := fixupImage(ctx, &original.BaseImage, relocationMap, cfg, events, cfg.componentImagePlatformFilter); 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.
I don't understand why the output from fixupImage
is sent to a separate goroutine in order to be printed to stderr. Since the calls to fixupImage
are executed serially, why can't it just print to stderr directly?
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.
As the fixup
will push or mount several images, we thought that having a more sophisticated event base mechanism will help the consumer of this library to create a good UX on this. For example with docker/app
we use https://github.com/morikuni/aec
to have a nice UI in the term. Does that make sense?
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 suppose so. The only issue I can think of immediately is if one of these event listeners blocks, it will hold up the termination of the whole process, but I guess that's ok. The user can always Ctrl-C.
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.
LGTM with a few minor comments/suggestions.
- Fixup does not mutate the bundle anymore, unless we want it too using auto-update-bundle, then it will update the digest, size and media type sections - Fixup now generates a relocation map, with original service/invocation images as keys and the digested references of the pushed images as values - Pull now generates the relocation map too. The result of pulling a bundle is a bundle.json and a relocation map. - Bundle is now stored as is in the registry, it is no more re-generated from the OCI index. Bundle is stored using the canonical form. - Split --output flag for fixup and pull commands into 2 flags: --bundle and --relocation-map - Harden e2e test, checking fixed bundle and generated relocation map Signed-off-by: Radu M <[email protected]> Signed-off-by: Silvin Lubecki <[email protected]>
541c483
to
ea06786
Compare
I addressed your comments @glyn, thank you 👍 |
closes #58
Changes in this PR:
push
, it can be safely dropped - not entirely sure if it's better to just not return it at all - WDYT?)bundle.json
file in canonical formbundle.json
in canonical formfixup
flag for automatically updating the bundle when media type, content digest, and size are missing from an image.Thanks to @silvin-lubecki for pairing on this!