Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Store repo digest as app image is pushed #770

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Nov 27, 2019

- What I did
Added repository digest as a stored info on app images (aka "bundles")

- How I did it
added a digest text file in bundlestore

- How to verify it
docker app image pull foo@sha256:123
docker app image push myapp

- Description for the changelog
Store App Image repository Digest

- A picture of a cute animal (not mandatory)
image

@@ -45,7 +45,7 @@ func runTag(bundleStore store.BundleStore, srcAppImage, destAppImage string) err
if err != nil {
return err
}

srcRef.RepoDigest = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand that correctly, tagging a pulled app image means we drop its repo digest? So it won't be displayed in the docker app image ls --digest column?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be wrong, but IIUC repo digest won't be the same if you push under a distinct tag. So keeing this "pulled digest" for a tagged image would be wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be the same repo digest, as tagging is just pushing a new descriptor, pointing to the same manifest/manifest list, but the repo digest should be the digest of the pointed object, not the pointer. cc @eunomie / @thaJeztah any idea?

also fix app image list --digests so it doesn't show "app build ID" as
digest

Signed-off-by: Nicolas De Loof <[email protected]>
Signed-off-by: Nicolas De Loof <[email protected]>
@codecov
Copy link

codecov bot commented Nov 30, 2019

Codecov Report

Merging #770 into master will decrease coverage by 0.89%.
The diff coverage is 68.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #770     +/-   ##
=========================================
- Coverage   71.32%   70.43%   -0.9%     
=========================================
  Files          67       67             
  Lines        3983     3768    -215     
=========================================
- Hits         2841     2654    -187     
+ Misses        792      764     -28     
  Partials      350      350
Impacted Files Coverage Δ
internal/cnab/cnab.go 32.3% <0%> (ø) ⬆️
internal/commands/build/build.go 59.8% <100%> (-6.6%) ⬇️
internal/store/digest.go 84% <100%> (-1.19%) ⬇️
internal/commands/image/tag.go 84.61% <100%> (ø) ⬆️
internal/commands/image/list.go 84.21% <100%> (-0.98%) ⬇️
internal/packager/bundle.go 64% <50%> (ø) ⬆️
internal/commands/push.go 34.53% <50%> (-3.35%) ⬇️
internal/store/bundle.go 76.72% <61.9%> (-8.39%) ⬇️
internal/relocated/bundle.go 60.75% <75%> (-7.76%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e244826...6d54f0c. Read the comment docs.

@ndeloof ndeloof marked this pull request as ready for review November 30, 2019 10:40
return errors.Wrapf(err, "could not push %q", ref)
}

// we can't just re-use bndl var here, as fixup did rewrite the bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

As stated in the specs, https://github.com/cnabio/cnab-spec/blob/master/101-bundle-json.md#invocation-images

During bundle development, it may be ideal to omit the contentDigest field and/or skip validation. Once a bundle is ready to be transmitted as a thick or thin bundle, it must have a contentDigest field.

the contentDigest can be omitted, but needs to be filled before pushing. I think that's exactly what we do here, that's why the bundle is mutated by the push.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants