Made the digest exporter report image digest if there is only one image. #1237

chhsia0 · 2019-08-23T04:43:55Z

Changes

Currently the image digest exporter does not implemented the behavior described
in resources.md, which says "if there are multiple versions of the image,
the latest will be used." Instead, it reports the digest of index.json, which
is an image index. This behavior introduces a usability issue: one of the major
public container registry --- dockerhub --- does not support OCI image indices,
and there are very few tools (if any) that support converting OCI image indices
to docker manifest lists. E.g., skopeo currently only support pushing an OCI image
index that contains only one image. If the index has more than one images, it
requires the user to specify one:
containers/skopeo#107
containers/image#400

Essentially, these limitations make the image digest exporter less useful. To make
it more usable, the exporter could instead implement the following behavior:

If there is only one image in index.json, report the image's digest.
If there are multiple images, report the digest of the full index.

The advantage of this behavior is that, we can immediately use it (in conjunction
of GoogleContainerTools/kaniko#744), yet if multi-image
manifests are more widely supported in the future, the image digest exporter can
still support that without any modification.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task and TaskRun to build and release this image

Reviewer Notes

If API changes
are included, additive changes
must be approved by at least two OWNERS
and backwards incompatible changes
must be approved by more than 50% of the OWNERS,
and they must first be added
in a backwards compatible way.

Release Notes

Although the exported digest changes for any single-image index, the the digests of either the index or the image are eventually used to refer to the image, so there should be no actual user impact.

…ne image. Currently the image digest exporter does not implemented the behavior described in the resources doc, which says "if there are multiple versions of the image, the latest will be used." Instead, it reports the digest of `index.json`, which is an image index. This behavior introduces a usability issue: one of the major public container registry --- dockerhub --- does not support OCI image indices, and there are very few tools (if any) that support converting OCI image indices to docker manifest lists. Skopeo currently only support pushing an OCI image index that contain only one image. If the index has more than one images, it requires the user to specify one: containers/skopeo#107 containers/image#400 Essentially, these limitations make the image digest exporter useless. To make this feature useful, the exporter could instead implement the following behavior: 1. If there is only one image in `index.json`, report the image's digest. 2. If there are multiple images, report the digest of the full index. The advantage of this behavior is that, we can immediately use it (in conjunction of GoogleContainerTools/kaniko#744), yet if multi-image manifests are more widely supported, the image digest exporter can still support that without any modification.

tekton-robot · 2019-08-23T04:44:10Z

Hi @chhsia0. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chhsia0 · 2019-08-23T17:24:45Z

/assign @dibyom

ghost · 2019-08-27T13:27:48Z

cmd/imagedigestexporter/main.go

@@ -55,9 +56,19 @@ func main() {
 log.Printf("ImageResource %s doesn't have an index.json file: %s", imageResource.Name, err)
 continue
 }
- digest, err := ii.Digest()
+ // If there is only one image in the index, report the digest of the image; otherwise, report the digest of the whole index.
+ digest, err := func() (v1.Hash, error) {


Suggest moving this to a package-level function, with a name, and passing ii as an argument to it.

and unit tests! :)

bobcatfish

+1 to what @sbwsg said, otherwise looks good! Thanks @chhsia0 :D

Also p.s. I think you've seen this already but note that we're probably going to change the way that resourceResults works pretty dramatically in the future (proposal to remove in 0.9 release)

bobcatfish · 2019-08-27T19:30:25Z

cmd/imagedigestexporter/main.go

@@ -55,9 +56,19 @@ func main() {
 log.Printf("ImageResource %s doesn't have an index.json file: %s", imageResource.Name, err)
 continue
 }
- digest, err := ii.Digest()
+ // If there is only one image in the index, report the digest of the image; otherwise, report the digest of the whole index.
+ digest, err := func() (v1.Hash, error) {


and unit tests! :)

afrittoli · 2019-08-28T20:41:50Z

/ok-to-test

chhsia0 · 2019-08-30T22:21:06Z

Sorry about the delay. Will add a test and fix the problems on Monday.

tekton-robot · 2019-09-04T15:16:54Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/imagedigestexporter/digest.go	Do not exist	83.3%
cmd/imagedigestexporter/main.go	Do not exist	0.0%

tekton-robot · 2019-09-04T18:28:34Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/imagedigestexporter/digest.go	Do not exist	83.3%
cmd/imagedigestexporter/main.go	Do not exist	0.0%

tekton-robot · 2019-09-05T02:28:41Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/imagedigestexporter/digest.go	Do not exist	83.3%
cmd/imagedigestexporter/main.go	Do not exist	0.0%

tekton-robot · 2019-09-05T04:29:04Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/imagedigestexporter/digest.go	Do not exist	71.4%
cmd/imagedigestexporter/main.go	Do not exist	0.0%

chhsia0 · 2019-09-05T04:50:55Z

Error message from the e2e test:

I0905 04:46:45.989] >>>> taskrun multiple-build-push-kaniko-run
I0905 04:46:49.188] Tailing logs for taskrun: multiple-build-push-kaniko-run...
...
I0905 04:46:49.316] [step-image-digest-exporter-n9qvv] Build Failed: 2019/09/05 04:45:27 Unexpected error getting image digest for skaffold-image-leeroy-web-2: {
I0905 04:46:49.316]   "schemaVersion": 2,
I0905 04:46:49.316]   "manifests": [
I0905 04:46:49.317]     {
I0905 04:46:49.317]       "mediaType": "application/vnd.oci.image.index.v1+json",
I0905 04:46:49.317]       "size": 314,
I0905 04:46:49.317]       "digest": "sha256:05f95b26ed10668b7183c1e2da98610e91372fa9f510046d4ce5812addad86b5"
... skipping 2 lines ...
I0905 04:46:49.317] }
I0905 04:46:49.317] EOF         
I0905 04:46:49.318] : invalid character 'E' after top-level value

Judging from the message, I suspect that the EOF comes from the following line:

pipeline/examples/taskruns/task-multiple-output-image.yaml

Line 107 in d8a2fa5

EOF

However I could not understand how the EOF ended up there. The block scalar YAML syntax and the shell's here document syntax looks correct to me.
@sbwsg @bobcatfish Could you take a look?

ghost · 2019-09-05T17:46:00Z

@chhsia0 I think it's the whitespace at the end of that line with the EOF. I don't know why your change has caused the image digest exporter to become sensitive to that but removing it locally and running the example on your branch works for me.

chhsia0 · 2019-09-05T18:29:44Z

🤦‍♂
@sbwsg Thank you very much! Should have looked at it more carefully 😅
My change makes it become sensitive because the manifest is now parsed.

tekton-robot · 2019-09-05T18:41:50Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/imagedigestexporter/digest.go	Do not exist	83.3%
cmd/imagedigestexporter/main.go	Do not exist	0.0%

ghost · 2019-09-05T18:42:02Z

Good stuff!

/lgtm

vdemeester

/lgtm

tekton-robot · 2019-09-06T07:16:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chhsia0, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Aug 23, 2019

tekton-robot requested review from abayer and bobcatfish August 23, 2019 04:43

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 23, 2019

tekton-robot assigned dibyom Aug 23, 2019

ghost reviewed Aug 27, 2019

View reviewed changes

bobcatfish reviewed Aug 27, 2019

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 28, 2019

Added a unit test.

8aabc3f

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 4, 2019

chhsia0 force-pushed the export-image-digest branch from 1f3e3c4 to f68448f Compare September 5, 2019 02:25

chhsia0 force-pushed the export-image-digest branch from 3cd05a8 to 1f62071 Compare September 5, 2019 04:26

Fixed e2e test errors.

78b21d4

chhsia0 force-pushed the export-image-digest branch from 1f62071 to 78b21d4 Compare September 5, 2019 18:38

tekton-robot assigned ghost Sep 5, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2019

chhsia0 requested a review from bobcatfish September 5, 2019 21:33

vdemeester approved these changes Sep 6, 2019

View reviewed changes

tekton-robot assigned vdemeester Sep 6, 2019

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2019

tekton-robot merged commit 1ac3262 into tektoncd:master Sep 6, 2019

chhsia0 mentioned this pull request Sep 6, 2019

Added --oci-layout-path flag to save image in OCI layout. GoogleContainerTools/kaniko#744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made the digest exporter report image digest if there is only one image. #1237

Made the digest exporter report image digest if there is only one image. #1237

chhsia0 commented Aug 23, 2019 •

edited

Loading

tekton-robot commented Aug 23, 2019

chhsia0 commented Aug 23, 2019

ghost Aug 27, 2019

bobcatfish Aug 27, 2019

bobcatfish left a comment

bobcatfish Aug 27, 2019

afrittoli commented Aug 28, 2019

chhsia0 commented Aug 30, 2019

tekton-robot commented Sep 4, 2019

tekton-robot commented Sep 4, 2019

tekton-robot commented Sep 5, 2019

tekton-robot commented Sep 5, 2019

chhsia0 commented Sep 5, 2019

ghost commented Sep 5, 2019

chhsia0 commented Sep 5, 2019

tekton-robot commented Sep 5, 2019

ghost commented Sep 5, 2019

vdemeester left a comment

tekton-robot commented Sep 6, 2019

Made the digest exporter report image digest if there is only one image. #1237

Made the digest exporter report image digest if there is only one image. #1237

Conversation

chhsia0 commented Aug 23, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Aug 23, 2019

chhsia0 commented Aug 23, 2019

ghost Aug 27, 2019

Choose a reason for hiding this comment

bobcatfish Aug 27, 2019

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Aug 27, 2019

Choose a reason for hiding this comment

afrittoli commented Aug 28, 2019

chhsia0 commented Aug 30, 2019

tekton-robot commented Sep 4, 2019

tekton-robot commented Sep 4, 2019

tekton-robot commented Sep 5, 2019

tekton-robot commented Sep 5, 2019

chhsia0 commented Sep 5, 2019

ghost commented Sep 5, 2019

chhsia0 commented Sep 5, 2019

tekton-robot commented Sep 5, 2019

ghost commented Sep 5, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 6, 2019

chhsia0 commented Aug 23, 2019 •

edited

Loading