Add support for comment and label manifests. #1408

wlynch · 2019-10-09T21:37:29Z

Changes

Introduces .MANIFEST files to comment and label directories to keep
track of sub-resources when the PullRequest resource is initialized.
This allows us to keep track of what resources the user explicitly
deleted, and not accidentally overwrite new comments/lables that were
introduced during execution.

Minor changes included:

Changed fake GitHub server to make copies of PullRequests to avoid
pointer collision across tests.
Changed uploadLabels to Add/Delete labels explicitly instead of
replacing all.

Change manifest path to .MANIFEST to avoid collisions.

Fixes #1286

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

- Fixes bug where PullRequestResource could accidentally delete newly created upsteam resources in certain race conditions.

tekton-robot · 2019-10-09T21:39:53Z

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/pullrequest-init/disk.go	75.9%	76.9%	0.9
cmd/pullrequest-init/fake_github.go	74.8%	73.3%	-1.5
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

tekton-robot · 2019-10-09T22:57: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
cmd/pullrequest-init/disk.go	75.9%	76.9%	0.9
cmd/pullrequest-init/fake_github.go	74.8%	73.3%	-1.5
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

wlynch · 2019-10-09T23:00:32Z

/hold

Found some issues with the manifest, so this is not ready for review yet. 😅

tekton-robot · 2019-10-09T23:14: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/pullrequest-init/disk.go	75.9%	77.0%	1.1
cmd/pullrequest-init/fake_github.go	74.8%	73.3%	-1.5
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

ghost

Mentioned some minor nits but otherwise this looks good to me!

cmd/pullrequest-init/disk.go

cmd/pullrequest-init/disk_test.go

cmd/pullrequest-init/fake_github.go

wlynch

Done! Will rebase and squash once I get an LGTM.

cmd/pullrequest-init/disk.go

cmd/pullrequest-init/disk_test.go

cmd/pullrequest-init/fake_github.go

wlynch · 2019-10-11T20:32:07Z

/hold cancel

tekton-robot · 2019-10-11T20:32:42Z

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/pullrequest-init/disk.go	75.9%	77.0%	1.1
cmd/pullrequest-init/fake_github.go	74.8%	72.7%	-2.1
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

ghost · 2019-10-14T14:12:45Z

/lgtm

Introduces `.MANIFEST` files to comment and label directories to keep track of sub-resources when the PullRequest resource is initialized. This allows us to keep track of what resources the user explicitly deleted, and not accidentally overwrite new comments/lables that were introduced during execution. Minor changes included: * Changed fake GitHub server to make copies of PullRequests to avoid pointer collision across tests. * Changed uploadLabels to Add/Delete labels explicitly instead of replacing all. Change manifest path to .MANIFEST to avoid collisions. Fixes tektoncd#1286

wlynch · 2019-10-14T16:31:30Z

Squashed the commits in prep for submission.

ghost · 2019-10-14T16:31:50Z

/lgtm

tekton-robot · 2019-10-14T16:33:12Z

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/pullrequest-init/disk.go	75.9%	77.0%	1.1
cmd/pullrequest-init/fake_github.go	74.8%	72.7%	-2.1
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

dlorenc · 2019-10-15T14:17:01Z

cmd/pullrequest-init/disk.go

 	for _, c := range comments {
-		commentPath := filepath.Join(path, strconv.FormatInt(c.ID, 10)+".json")
+		id := strconv.FormatInt(c.ID, 10)


Is this manifest intended to be human readable? Would json serializing or gob serializing make it easier to safely round trip, so we don't have to manually encode to text?

Great idea!

Manifests are not meant to be human readable. Switched to gob encoding.

tekton-robot · 2019-10-15T18:01:38Z

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/pullrequest-init/disk.go	75.9%	78.2%	2.2
cmd/pullrequest-init/fake_github.go	74.8%	72.7%	-2.1
cmd/pullrequest-init/github.go	83.5%	82.5%	-1.1

ghost · 2019-10-15T19:13:01Z

/lgtm

dlorenc · 2019-10-15T20:23:11Z

/approve

tekton-robot · 2019-10-15T20:23:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc

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

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

tekton-robot requested review from dlorenc and a user October 9, 2019 21:37

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

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

wlynch force-pushed the manifest branch from 83e9950 to 9dd6c48 Compare October 9, 2019 22:55

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2019

ghost reviewed Oct 10, 2019

View reviewed changes

cmd/pullrequest-init/disk.go Outdated Show resolved Hide resolved

cmd/pullrequest-init/disk_test.go Outdated Show resolved Hide resolved

cmd/pullrequest-init/fake_github.go Show resolved Hide resolved

wlynch commented Oct 11, 2019

View reviewed changes

cmd/pullrequest-init/disk.go Outdated Show resolved Hide resolved

cmd/pullrequest-init/disk_test.go Outdated Show resolved Hide resolved

cmd/pullrequest-init/fake_github.go Show resolved Hide resolved

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2019

tekton-robot assigned ghost Oct 14, 2019

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

wlynch force-pushed the manifest branch from 50c2e14 to dddd6f1 Compare October 14, 2019 16:31

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

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

dlorenc reviewed Oct 15, 2019

View reviewed changes

Use gob encoding for manifest files.

798b932

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

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

dlorenc approved these changes Oct 15, 2019

View reviewed changes

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

tekton-robot merged commit 9e10c01 into tektoncd:master Oct 15, 2019

vtereso mentioned this pull request Oct 18, 2019

Comments deleted on pull request by monitor task tektoncd/experimental#221

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for comment and label manifests. #1408

Add support for comment and label manifests. #1408

wlynch commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

wlynch commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

ghost left a comment

wlynch left a comment

wlynch commented Oct 11, 2019

tekton-robot commented Oct 11, 2019

ghost commented Oct 14, 2019

wlynch commented Oct 14, 2019

ghost commented Oct 14, 2019

tekton-robot commented Oct 14, 2019

dlorenc Oct 15, 2019

wlynch Oct 15, 2019

tekton-robot commented Oct 15, 2019

ghost commented Oct 15, 2019

dlorenc commented Oct 15, 2019

tekton-robot commented Oct 15, 2019

Add support for comment and label manifests. #1408

Add support for comment and label manifests. #1408

Conversation

wlynch commented Oct 9, 2019

Changes

Submitter Checklist

Reviewer Notes

Release Notes

tekton-robot commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

wlynch commented Oct 9, 2019

tekton-robot commented Oct 9, 2019

ghost left a comment

Choose a reason for hiding this comment

wlynch left a comment

Choose a reason for hiding this comment

wlynch commented Oct 11, 2019

tekton-robot commented Oct 11, 2019

ghost commented Oct 14, 2019

wlynch commented Oct 14, 2019

ghost commented Oct 14, 2019

tekton-robot commented Oct 14, 2019

dlorenc Oct 15, 2019

Choose a reason for hiding this comment

wlynch Oct 15, 2019

Choose a reason for hiding this comment

tekton-robot commented Oct 15, 2019

ghost commented Oct 15, 2019

dlorenc commented Oct 15, 2019

tekton-robot commented Oct 15, 2019