Refactor Resource result output, and add support for Git resources. #1424

dlorenc · 2019-10-14T15:07:41Z

Changes

This change builds upon #1406, and logs the exact Git commit used by a Git input
to the ResourceResults field.

Some other cleanups are included:

Removing custom TerminationMessagePath from the Image resource. The default is fine.
Test cleanup.
A new helper to write termination messages from resource containers.

And finally, this adds a new environment variable to the git resource steps, indicating the
name of the resource instance they are running as. We should make this more generic and apply
it to all resource steps as part of the extensiblity refactor.

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

The 'Git' PipelineResource now populates the taskRun.status.resourcesResult field with the commit used.

tekton-robot · 2019-10-14T15:09: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
pkg/reconciler/taskrun/resources/image_exporter.go	87.5%	86.2%	-1.3
pkg/reconciler/taskrun/taskrun.go	71.6%	73.6%	1.9
test/builder/container.go	86.8%	91.7%	4.8

tekton-robot · 2019-10-14T17:39:07Z

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/reconciler/taskrun/resources/image_exporter.go	87.5%	86.2%	-1.3
pkg/reconciler/taskrun/taskrun.go	71.6%	73.6%	1.9
test/builder/container.go	86.8%	91.7%	4.8

ghost

/lgtm

some minor nits but i don't see any showstoppers.

cmd/git-init/main.go

docs/resources.md

ghost · 2019-10-16T13:21:26Z

pkg/git/git.go

 	c := exec.Command("git", args...)
 	var output bytes.Buffer
 	c.Stderr = &output
 	c.Stdout = &output
+	if dir != "" {
+		c.Dir = dir


nit: I'm a bit unclear why the assignment is wrapped in an if. What is c.Dirat this point and what are we protecting against with the if?

Clarified with a comment.

test/kaniko_task_test.go

dlorenc · 2019-10-16T14:11:38Z

some minor nits but i don't see any showstoppers.

Thanks! Ready for another look.

tekton-robot · 2019-10-16T14:13:06Z

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/reconciler/taskrun/resources/image_exporter.go	87.5%	86.2%	-1.3
pkg/reconciler/taskrun/taskrun.go	71.6%	73.6%	1.9
test/builder/container.go	86.8%	91.7%	4.8

ghost · 2019-10-16T14:36:12Z

/lgtm

dibyom

one tiny nit otherwise LGTM!

cmd/termination/termination.go

tekton-robot · 2019-10-16T17:40: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/reconciler/taskrun/resources/image_exporter.go	87.5%	86.2%	-1.3
pkg/reconciler/taskrun/taskrun.go	71.6%	73.6%	1.9
test/builder/container.go	86.8%	91.7%	4.8

dibyom

/lgtm

vdemeester

@dlorenc thanks, looks good, just one question on the cmd/termination package 👼

vdemeester · 2019-10-17T09:15:53Z

cmd/termination/termination.go

+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package termination


Any reason for this package being in cmd/* instead of pkg/* ?
Usually we make the assumption that any package cmd/foo produce a binary called foo, which is not the case for this one (as there is no main).

This is basically a library only for command binaries so I was trying to colocate it here, but it seems like that's a little unclear. I'll move it.

This change builds upon tektoncd#1406, and logs the exact Git commit used by a Git input to the ResourceResults field. Some other cleanups are included: - Removing custom TerminationMessagePath from the Image resource. The default is fine. - Test cleanup. - A new helper to write termination messages from resource containers. And finally, this adds a new environment variable to the git resource steps, indicating the name of the resource instance they are running as. We should make this more generic and apply it to all resource steps as part of the extensiblity refactor.

tekton-robot · 2019-10-17T13:41: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/reconciler/taskrun/resources/image_exporter.go	87.5%	86.2%	-1.3
pkg/reconciler/taskrun/taskrun.go	70.5%	72.4%	1.9
test/builder/container.go	86.8%	91.7%	4.8

dlorenc · 2019-10-17T13:57:31Z

/test pull-tekton-pipeline-build-tests

ghost · 2019-10-17T14:02:27Z

/lgtm

vdemeester

/lgtm

tekton-robot · 2019-10-17T14:07:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 Oct 14, 2019

tekton-robot requested review from abayer and bobcatfish October 14, 2019 15:07

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 14, 2019

dlorenc force-pushed the termgit branch from d8a5932 to 61c3a75 Compare October 14, 2019 17:37

ghost reviewed Oct 16, 2019

View reviewed changes

tekton-robot assigned ghost Oct 16, 2019

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

dlorenc force-pushed the termgit branch from 61c3a75 to 6ebc6db Compare October 16, 2019 14:10

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

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

dibyom reviewed Oct 16, 2019

View reviewed changes

cmd/termination/termination.go Outdated Show resolved Hide resolved

dlorenc force-pushed the termgit branch from 6ebc6db to 52c958b Compare October 16, 2019 17:38

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

dibyom reviewed Oct 16, 2019

View reviewed changes

tekton-robot assigned dibyom Oct 16, 2019

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

vdemeester requested changes Oct 17, 2019

View reviewed changes

dlorenc force-pushed the termgit branch from 52c958b to 59617fa Compare October 17, 2019 13:39

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

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

vdemeester approved these changes Oct 17, 2019

View reviewed changes

tekton-robot assigned vdemeester Oct 17, 2019

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

tekton-robot merged commit ea94852 into tektoncd:master Oct 17, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Resource result output, and add support for Git resources. #1424

Refactor Resource result output, and add support for Git resources. #1424

dlorenc commented Oct 14, 2019 •

edited

Loading

tekton-robot commented Oct 14, 2019

tekton-robot commented Oct 14, 2019

ghost left a comment

ghost Oct 16, 2019

dlorenc Oct 16, 2019

dlorenc commented Oct 16, 2019

tekton-robot commented Oct 16, 2019

ghost commented Oct 16, 2019

dibyom left a comment

tekton-robot commented Oct 16, 2019

dibyom left a comment

vdemeester left a comment

vdemeester Oct 17, 2019

dlorenc Oct 17, 2019

tekton-robot commented Oct 17, 2019

dlorenc commented Oct 17, 2019

ghost commented Oct 17, 2019

vdemeester left a comment

tekton-robot commented Oct 17, 2019

Refactor Resource result output, and add support for Git resources. #1424

Refactor Resource result output, and add support for Git resources. #1424

Conversation

dlorenc commented Oct 14, 2019 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Oct 14, 2019

tekton-robot commented Oct 14, 2019

ghost left a comment

Choose a reason for hiding this comment

ghost Oct 16, 2019

Choose a reason for hiding this comment

dlorenc Oct 16, 2019

Choose a reason for hiding this comment

dlorenc commented Oct 16, 2019

tekton-robot commented Oct 16, 2019

ghost commented Oct 16, 2019

dibyom left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 16, 2019

dibyom left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

vdemeester Oct 17, 2019

Choose a reason for hiding this comment

dlorenc Oct 17, 2019

Choose a reason for hiding this comment

tekton-robot commented Oct 17, 2019

dlorenc commented Oct 17, 2019

ghost commented Oct 17, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Oct 17, 2019

dlorenc commented Oct 14, 2019 •

edited

Loading