Change the path used by the image digest exporter to the standard out… #1467

dlorenc · 2019-10-24T18:59:33Z

…put path structure.

Previously we were writing output digests to /builder/home/image-output-paths. This commit
unifies that system and expects tasks to output these files to /workspace/output/.

This does introduce a change in behavior, but support is retained for the old location. A warning
is logged if the binary finds an index.json in the old location.

Changes

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 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

Image index.json files are now expected to be written to /workspace/output/<image resource name> instead of /builder/home/image-outputs. The old location is still supported, but support for this will be removed in a future release.

tekton-robot · 2019-10-24T19:01:46Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

dlorenc · 2019-10-24T19:02:12Z

cc @imjasonh this will probably conflict with your #1440, but it should make things easier in the long run

bobcatfish

I think we need a docs update probs, and I'm a bit confused, are these docs wrong https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#surfacing-the-image-digest-built-in-a-task which way we default to /workspace/output/{resource-name} or are we talking about 2 different paths?

bobcatfish · 2019-10-24T19:03:25Z

pkg/reconciler/taskrun/resources/pod_test.go

-			Name:         "create-dir-default-image-output-mz4c7",
-			Image:        "override-with-bash-noop:latest",
-			Command:      []string{"/ko-app/bash"},
-			Args:         []string{"-args", "mkdir -p /builder/home/image-outputs/outputimage"},


question: do we not still want this test but with the new path? or do you we feel like this is already covered somewhere else/

The function that this was testing got deleted, it now uses the normal output directory creation stuff, which is tested: https://github.com/tektoncd/pipeline/blob/master/pkg/reconciler/taskrun/resources/output_resource_test.go#L171

dlorenc · 2019-10-24T20:10:35Z

I think we need a docs update probs, and I'm a bit confused, are these docs wrong https://github.com/tektoncd/pipeline/blob/master/docs/resources.md#surfacing-the-image-digest-built-in-a-task which way we default to /workspace/output/{resource-name} or are we talking about 2 different paths?

Wow, those docs are currently wrong then, and I guess my change makes them right?

tekton-robot · 2019-10-24T20:15:20Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

tekton-robot · 2019-10-24T20:15:35Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

tekton-robot · 2019-10-24T20:40:49Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

tekton-robot · 2019-10-25T14:59:52Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

ghost · 2019-10-28T15:15:56Z

/test pull-tekton-pipeline-integration-tests

cmd/imagedigestexporter/main.go

ghost · 2019-10-28T15:19:41Z

pkg/apis/pipeline/v1alpha1/task_types.go

@@ -124,7 +119,6 @@ type Inputs struct {
 type TaskResource struct {
 	ResourceDeclaration `json:",inline"`
 	// +optional
-	OutputImageDir string `json:"outputImageDir,omitempty"`


This is an API change, right? Would it be worth supporting this field in a deprecated state for a version?

So it's technically on the API, but as you can see up in image_resource.go it is never actually set during the initialization of the resource. I don't think it's actually functional right now.

It's technically part of the API, but I think it's unused. You can see that this value is never copied from the resource params into the resource in the constructor in image_resource.go.

This is causing a issue, because we do have resources with outputImageDir and they cannot be updated now :(

pkg/apis/pipeline/v1alpha1/task_types.go

…put path structure. Previously we were writing output digests to /builder/home/image-output-paths. This commit unifies that system and expects tasks to output these files to /workspace/output/<resource name>. This does introduce a change in behavior, but support is retained for the old location. A warning is logged if the binary finds an index.json in the old location.

tekton-robot · 2019-10-28T17:39:47Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

tekton-robot · 2019-10-28T17:42:17Z

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
pkg/apis/pipeline/v1alpha1/image_resource.go	56.2%	60.0%	3.8
pkg/apis/pipeline/v1alpha1/task_defaults.go	90.0%	80.0%	-10.0
pkg/apis/pipeline/v1alpha1/task_validation.go	81.2%	81.5%	0.3
pkg/reconciler/taskrun/resources/image_exporter.go	86.2%	83.3%	-2.9
pkg/reconciler/taskrun/resources/pod.go	91.3%	90.7%	-0.6

ghost · 2019-10-28T17:48:39Z

/lgtm

bobcatfish · 2019-10-29T20:07:34Z

/approve

tekton-robot · 2019-10-29T20:07:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

This support was left in for one release after deprecation in tektoncd#1467, which was released in 0.9.0. It can now be safely removed.

This support was left in for one release after deprecation in #1467, which was released in 0.9.0. It can now be safely removed.

tekton-robot requested review from abayer and bobcatfish October 24, 2019 18:59

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

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 24, 2019

bobcatfish reviewed Oct 24, 2019

View reviewed changes

dlorenc force-pushed the fiximgdgst branch 2 times, most recently from 7b7a0af to e801460 Compare October 24, 2019 20:13

dlorenc force-pushed the fiximgdgst branch from e801460 to 38795ab Compare October 24, 2019 20:38

dlorenc force-pushed the fiximgdgst branch from 38795ab to 7601658 Compare October 25, 2019 14:57

ghost reviewed Oct 28, 2019

View reviewed changes

dlorenc force-pushed the fiximgdgst branch from 7601658 to 1b30c97 Compare October 28, 2019 17:37

dlorenc force-pushed the fiximgdgst branch from 1b30c97 to 98be5d3 Compare October 28, 2019 17:40

tekton-robot assigned ghost Oct 28, 2019

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

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

tekton-robot merged commit ee806d2 into tektoncd:master Oct 29, 2019

dlorenc added a commit to dlorenc/build-pipeline that referenced this pull request Dec 3, 2019

Remove the support for legacy image output paths.

b81bc80

This support was left in for one release after deprecation in tektoncd#1467, which was released in 0.9.0. It can now be safely removed.

dlorenc mentioned this pull request Dec 3, 2019

Remove the support for legacy image output paths. #1671

Merged

3 tasks

tekton-robot pushed a commit that referenced this pull request Dec 3, 2019

Remove the support for legacy image output paths.

89eacb0

This support was left in for one release after deprecation in #1467, which was released in 0.9.0. It can now be safely removed.

afrittoli mentioned this pull request Dec 10, 2019

Cannot update Task with outputImageDir #1717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the path used by the image digest exporter to the standard out… #1467

Change the path used by the image digest exporter to the standard out… #1467

dlorenc commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

dlorenc commented Oct 24, 2019

bobcatfish left a comment

bobcatfish Oct 24, 2019

dlorenc Oct 24, 2019

dlorenc commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 25, 2019

ghost commented Oct 28, 2019

ghost Oct 28, 2019

dlorenc Oct 28, 2019

dlorenc Oct 28, 2019

afrittoli Dec 10, 2019

tekton-robot commented Oct 28, 2019

tekton-robot commented Oct 28, 2019

ghost commented Oct 28, 2019

bobcatfish commented Oct 29, 2019

tekton-robot commented Oct 29, 2019

Change the path used by the image digest exporter to the standard out… #1467

Change the path used by the image digest exporter to the standard out… #1467

Conversation

dlorenc commented Oct 24, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Oct 24, 2019

dlorenc commented Oct 24, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Oct 24, 2019

Choose a reason for hiding this comment

dlorenc Oct 24, 2019

Choose a reason for hiding this comment

dlorenc commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 24, 2019

tekton-robot commented Oct 25, 2019

ghost commented Oct 28, 2019

ghost Oct 28, 2019

Choose a reason for hiding this comment

dlorenc Oct 28, 2019

Choose a reason for hiding this comment

dlorenc Oct 28, 2019

Choose a reason for hiding this comment

afrittoli Dec 10, 2019

Choose a reason for hiding this comment

tekton-robot commented Oct 28, 2019

tekton-robot commented Oct 28, 2019

ghost commented Oct 28, 2019

bobcatfish commented Oct 29, 2019

tekton-robot commented Oct 29, 2019