Add metadata in imgpkg lock file #132

cari-lynn · 2021-07-22T22:21:21Z

When using the --imgpkg-lock-output flag, this PR adds the kbld.carvel.dev/metas annotation.

images:
- annotations:
    kbld.carvel.dev/metas: |
      - Tag: x.x.x
        Type: resolved
        URL: image-name:x.x.x

When using a lock file as input to kbld, this PR also will preserve any kbld.carvel.dev/metas. It will append new metas to the list in the output and an imgpkg Lock file. This results in a history for example:

images:
- annotations:
    kbld.carvel.dev/metas: |
      - Tag: x.x.x
        Type: resolved
        URL: image-name:x.x.x
      - Type: preresolved
        URL: index.docker.io/library/..@sha256:...

Other new behavior:

If a kbld.carvel.dev/metas annotation is malformatted, kbld will error.
the kbld.carvel.dev/metas annotation must be in the format of several predefined objects, other unknown meta objects will be ignored.

gcheadle-vmware · 2021-08-30T16:28:50Z

@cari-lynn Requesting review, was unable to select you as a GH reviewer.

pkg/kbld/config/config.go

pkg/kbld/image/preresolved.go

pkg/kbld/cmd/resolve.go

pkg/kbld/config/config.go

pkg/kbld/image/preresolved.go

test/e2e/lock_output_test.go

gcheadle-vmware · 2021-09-09T20:38:46Z

This looks good to me. Great idea extracting out the meta interface into its own package, it really cleaned up nicely :)

pivotaljohn

This work is really close.

The overall approach looks really good to me.

Details in the comments, below.

pkg/kbld/cmd/resolve.go

pkg/kbld/image/resolved.go

pkg/kbld/image/tag_selected.go

pkg/kbld/image/tagged.go

pkg/kbld/config/image_meta.go

pivotaljohn · 2021-09-13T23:48:04Z

Reviewed w/ @garrettcheadle (offering to make changes if desired):

i think that all suggestions but the last comment are simple changes that I see as improvements. ... If you could make those changes/renames, that would move this along.

👍🏻

In the case of "deliberately ignoring unmarshallable data" ... we have another custom unmarshaller in kbld, maybe we should check if that unmarshaller has an error case for wisdom?

I'll look into that.

pivotaljohn · 2021-09-14T00:02:32Z

In terms of prior art, I'm seeing this idiom which attempts to unmarshal data into a set of potential structs...

https://github.com/vmware-tanzu/carvel-kbld/blob/f919f7f78a/pkg/kbld/resources/path.go#L78-L91

By the end of the switch, if no suitable struct was found, that was reported as an error.

I think we should do a similar thing in the case of detecting which kind of Meta (if you can successfully unmarshall it, must be that kind!)... but also either emit a warning or error out when it's unrecognized.

cppforlife

(looks like i had review that wasnt submitted. might be a bit stale.)

pkg/kbld/image/preresolved.go

pkg/kbld/config/image_meta.go

pkg/kbld/config/config.go

pkg/kbld/cmd/resolve.go

pivotaljohn · 2021-09-20T17:00:57Z

Reviewed w/ @cppforlife over the phone.

UniqueImageOverrides is not needed when preparing to output imgpkg-flavored lock files. The other callers are in deprecated code. We'll remove this invocation from "resolve"
to minimize the impact of this change on working ocde, we'll fix the compilation error, by writing a custom Equals() that ignores metas in the comparison. 👍🏻

Signed-off-by: Cari Dean <cdean@pivotal.io>

Signed-off-by: Garrett Cheadle <gcheadle@vmware.com>

- exclude metas in imgpkg lock when image is given in digest form Signed-off-by: Cari Dean <cdean@pivotal.io>

- return err from unmarshalling image lock

Signed-off-by: Garrett Cheadle <gcheadle@vmware.com>

- Use custom unmarshaler for image metas - Move Metas from image package to config package - Rename Metas to ImageMetas

- extract meta type names as constants - fix slice append gotcha: when creating a new slice, copy from old first (i.e. copyAndAppendMeta()) - various renames and inlining to improve clarity

- meta are descriptive, not identifying, so they have no bearing on whether two ImageOverrides are equal. Given that all overrides are created brand fresh during a resolve for the kbld lock file, there's no need to dedup them (removed call to UniqueImageOverride()).

vmwclabot added the cla-not-required label Jul 22, 2021

gcheadle-vmware force-pushed the tag-metas branch 2 times, most recently from e43b985 to debe17b Compare August 26, 2021 16:08

cari-lynn commented Aug 30, 2021

View reviewed changes

pkg/kbld/config/config.go Outdated Show resolved Hide resolved

pkg/kbld/image/preresolved.go Outdated Show resolved Hide resolved

cppforlife reviewed Aug 30, 2021

View reviewed changes

gcheadle-vmware force-pushed the tag-metas branch from f02e03b to c553f0b Compare September 1, 2021 23:32

cari-lynn force-pushed the tag-metas branch from 2e7eeff to e9b779f Compare September 8, 2021 19:01

cari-lynn changed the title ~~Add tag metadata in imgpkg lock file~~ Add metadata in imgpkg lock file Sep 8, 2021

cari-lynn marked this pull request as ready for review September 8, 2021 19:24

pivotaljohn suggested changes Sep 13, 2021

View reviewed changes

cppforlife reviewed Sep 14, 2021

View reviewed changes

pivotaljohn had a problem deploying to DockerHub E2E September 17, 2021 21:38 Failure

pivotaljohn approved these changes Sep 17, 2021

View reviewed changes

pivotaljohn force-pushed the tag-metas branch from 8aed0e9 to f593752 Compare September 17, 2021 21:45

pivotaljohn temporarily deployed to DockerHub E2E September 17, 2021 21:45 Inactive

pivotaljohn requested a review from cppforlife September 17, 2021 22:20

pivotaljohn temporarily deployed to DockerHub E2E September 20, 2021 22:34 Inactive

cari-lynn and others added 8 commits September 20, 2021 15:41

Add kbld.carvel.dev/metas in imgpkg lock file

ceef594

Include image metas in output when provided by imgpkg lock file

b65ffa2

Signed-off-by: Cari Dean <cdean@pivotal.io>

Update failing test expectations and fix typos

757d29b

Signed-off-by: Garrett Cheadle <gcheadle@vmware.com>

Include 'Type: preresolved' as metadata in imgpkg lock file

cdfbb52

- exclude metas in imgpkg lock when image is given in digest form Signed-off-by: Cari Dean <cdean@pivotal.io>

Change func to receiver

92205f2

- return err from unmarshalling image lock

Add preresolved meta to image metas from imgpkg lock

8c5a1bc

Add all image meta fields to ImageMeta struct

589b89c

Signed-off-by: Garrett Cheadle <gcheadle@vmware.com>

Unmarshal ImageMetas from Lock file in config

651c3f1

- Use custom unmarshaler for image metas - Move Metas from image package to config package - Rename Metas to ImageMetas

cari-lynn and others added 3 commits September 20, 2021 15:41

Add license header :)

424e00b

Reinitalize Metas for every item

05a328a

Fit and finish from review

1ad081d

- extract meta type names as constants - fix slice append gotcha: when creating a new slice, copy from old first (i.e. copyAndAppendMeta()) - various renames and inlining to improve clarity

pivotaljohn force-pushed the tag-metas branch from dbc497e to 9e718e6 Compare September 20, 2021 22:41

pivotaljohn temporarily deployed to DockerHub E2E September 20, 2021 22:41 Inactive

pivotaljohn force-pushed the tag-metas branch from 9e718e6 to 2c48ce6 Compare September 20, 2021 23:00

pivotaljohn temporarily deployed to DockerHub E2E September 20, 2021 23:00 Inactive

cppforlife merged commit 517a578 into develop Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metadata in imgpkg lock file #132

Add metadata in imgpkg lock file #132

cari-lynn commented Jul 22, 2021 •

edited

Loading

gcheadle-vmware commented Aug 30, 2021

gcheadle-vmware commented Sep 9, 2021

pivotaljohn left a comment

pivotaljohn commented Sep 13, 2021

pivotaljohn commented Sep 14, 2021

cppforlife left a comment

pivotaljohn commented Sep 20, 2021 •

edited

Loading

Add metadata in imgpkg lock file #132

Add metadata in imgpkg lock file #132

Conversation

cari-lynn commented Jul 22, 2021 • edited Loading

gcheadle-vmware commented Aug 30, 2021

gcheadle-vmware commented Sep 9, 2021

pivotaljohn left a comment

Choose a reason for hiding this comment

pivotaljohn commented Sep 13, 2021

pivotaljohn commented Sep 14, 2021

cppforlife left a comment

Choose a reason for hiding this comment

pivotaljohn commented Sep 20, 2021 • edited Loading

cari-lynn commented Jul 22, 2021 •

edited

Loading

pivotaljohn commented Sep 20, 2021 •

edited

Loading