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

Store entire bundle, generate relocation map #65

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

radu-matei
Copy link
Member

@radu-matei radu-matei commented Aug 20, 2019

closes #58

Changes in this PR:

  • generate relocation map on both push and pull (regarding @glyn's comment about generating the map on push, it can be safely dropped - not entirely sure if it's better to just not return it at all - WDYT?)
  • store the entire bundle.json file in canonical form
  • pull bundle.json in canonical form
  • add a fixup 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!

if len(b.InvocationImages) == 0 {
return nil, fmt.Errorf("unknown invocation image: %q", d.Digest)
}
relocationMap[b.InvocationImages[0].Image] = refFamiliar
Copy link
Member Author

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.

Copy link

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?

@radu-matei radu-matei marked this pull request as ready for review August 28, 2019 20:07
@silvin-lubecki silvin-lubecki force-pushed the relocation-map branch 2 times, most recently from 7b5c27b to 45fdf5a Compare September 10, 2019 15:36
Copy link
Member Author

@radu-matei radu-matei left a 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!

@silvin-lubecki
Copy link
Collaborator

PTAL @glyn 🙏

converter/convert.go Outdated Show resolved Hide resolved
relocation/types.go Outdated Show resolved Hide resolved
remotes/fixup.go Outdated Show resolved Hide resolved
}
// 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 {
Copy link

@glyn glyn Sep 11, 2019

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?

Copy link
Collaborator

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?

Copy link

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.

Copy link

@glyn glyn left a 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]>
@silvin-lubecki
Copy link
Collaborator

I addressed your comments @glyn, thank you 👍

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.

Store the whole Bundle.json and generate a Relocation map
4 participants