Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add read-only copy of pull request object in resource output. #1685

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

wlynch
Copy link
Member

@wlynch wlynch commented Dec 4, 2019

Changes

Initial fix to grant users access to the rest of the PR information.
This file is read-only so that we can use the subresources (e.g. labels,
comments, etc.) as the source of truth.

As a follow up, we may want to consider pruning unneeded fields from the PR output.
I wasn't sure how to do this without forking the scm.PullRequest
resource, and I'd like to avoid this in order to make updates from the
upstream scm library as easy/minimal as possible. This should be fine for now to
get into for 0.9.1, and we can iterate on this as needed.

Fixes #1681

/cc @afrittoli

Submitter Checklist

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

See the contribution guide for more details.

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

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

PullRequestResource: Adds read-only pr.json to include other various pull request information.

@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Dec 4, 2019
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 4, 2019
@wlynch
Copy link
Member Author

wlynch commented Dec 4, 2019

/test pull-tekton-pipeline-integration-tests

@@ -0,0 +1 @@
{"Ref":"master","Sha":"723b9a9d560bdf4dc8fc6f697d53f662d3454ac8","Repo":{"ID":"146641150","Namespace":"tektoncd","Name":"pipeline","FullName":"tektoncd/pipeline","Perm":{"Pull":false,"Push":false,"Admin":false},"Branch":"master","Private":false,"Clone":"https://github.com/tektoncd/pipeline.git","CloneSSH":"[email protected]:tektoncd/pipeline.git","Link":"https://github.com/tektoncd/pipeline","Created":"2018-08-29T18:21:55Z","Updated":"2019-12-04T17:26:42Z"}}
Copy link
Member

Choose a reason for hiding this comment

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

I see a lot of JSON files under pullrequest-init/example...is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! Instead of embedding each payload in the resource docs, I figured it'd be more useful to include a complete example.

Copy link
Member

Choose a reason for hiding this comment

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

nit: we could use ./testdata folder if we want to use those for tests (it's a special folder in the go world)

Copy link
Member

Choose a reason for hiding this comment

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

yeah along the same lines, if this is just for documentation purposes, maybe there is a better folder than inside cmd. And if its for tests, maybe a testdata. But this is a pretty minor thing so that should not hold up this PR!

docs/resources.md Outdated Show resolved Hide resolved
Also fixes minor permission issue (rwx -> rw for most files).

Initial fix to grant users access to the rest of the PR information.
This file is read-only so that we can use the subresources (e.g. labels,
comments, etc.) as the source of truth.

As a follow up, we may want to consider pruning this from the PR output.
I wasn't sure how to do this without forking the scm.PullRequest
resource. I'd like to avoid this in order to make updates to the
upstream scm library as easy as possible. This should be fine for now to
get into for 0.9.1, and we can iterate on this as needed.
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2019
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm
/cc @afrittoli

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2019
@tekton-robot tekton-robot merged commit 6620822 into tektoncd:master Dec 5, 2019
@afrittoli
Copy link
Member

Thanks! We now need to cherry pick this one on branchv0.9.x, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0.9.0 Regression: PullRequestResource missing base PR information
6 participants