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 pull request template to integrate with prow release-note plugin #585

Merged
merged 1 commit into from Feb 15, 2021

Conversation

gabemontero
Copy link
Member

@gabemontero gabemontero commented Feb 11, 2021

To mimic upstream tekton on how they curate release notes on a per PR basis using the prow release-note plugin

Related PR: openshift/release#15770

/assign @adambkaplan

/assign @qu1queee

@sbose78 FYI

ultimately what happens is a release-note or release-note-none label is required for PR merge, and which label is applied is triggered by what is placed in the release-note description of the PR.

@gabemontero
Copy link
Member Author

gabemontero commented Feb 12, 2021

FYI openshift/release#15770 was merged sooner than I had intended (the test platform guys merged it when I was only trying to get them to add the approved label)

Short term, I've added the release-note-none label so PRs can still merge prior to landing some form of this PR. Or folks can manually add the release-note block per https://git.k8s.io/community/contributors/guide/release-notes.md


# Submitter Checklist

See [the contribution guide](https://github.com/shipwright-io/build/blob/master/CONTRIBUTING.md)
Copy link
Member

Choose a reason for hiding this comment

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

nit: /s/contribution/contributor/

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also be in an HTML comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

switched to contributor in next push

Copy link
Member Author

Choose a reason for hiding this comment

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

it was not a HTML comment in the the version in upstream Tekton I borrowed from @adambkaplan

See https://raw.githubusercontent.com/tektoncd/triggers/master/.github/pull_request_template.md
for example

Copy link
Member Author

Choose a reason for hiding this comment

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

but if the majority is to break from with that pattern and make this an HTML comment I'll defer and update accordingly

Copy link
Member

Choose a reason for hiding this comment

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

Given the title is "Submitter checklist", I would expect there to be an actual checklist that contributors could mark off, such as:

- [ ] Includes tests if functionality changed/was added
- [ ] Includes docs if changes are user-facing
- [ ] Release notes block has been filled in, or marked NONE

Copy link
Member Author

Choose a reason for hiding this comment

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

check list added @adambkaplan thanks

.github/pull_request_template.md Show resolved Hide resolved
@gabemontero
Copy link
Member Author

pushed everything besides the HTML comment note @adambkaplan (while that is still a pending discussion)

@gabemontero
Copy link
Member Author

Validations of the plugin via #590 look good

  • do-not-merge label shows up by default
  • if a release-note-none label is added the do-not-merge label is removed and tide does not block
  • you cannot add the release-note label manually without having the appropriate block in the description
  • once you add the release-note block in the description, the release-note label is added and any other related labels are removed

@gabemontero
Copy link
Member Author

Also tried to explore some github action variants. The closest thing I could find was https://github.com/marketplace/actions/pull-request-updater-github-action

Maybe it could accomplish what we need (though I could not say for sure). The pre-action thing here seem much more simpler / direct.

Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

looks good, just a minor typo from my side that needs to be fixed.

.github/pull_request_template.md Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
Copy link
Contributor

@qu1queee qu1queee left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
@gabemontero
Copy link
Member Author

gabemontero commented Feb 12, 2021

there is a lot of tl;dr discussion in https://kubernetes.slack.com/archives/C019ZRGUEJC/p1613143894064100
including details on how tekton processes the release-note and kind labels, as well as some possible github action stuff down the line to try

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
@gabemontero
Copy link
Member Author

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gabemontero

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021
Copy link
Member

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2021
@gabemontero
Copy link
Member Author

can we re-run the travis job if it flakes with an unrelated error?

@gabemontero
Copy link
Member Author

can we re-run the travis job if it flakes with an unrelated error?

found https://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit ... seeing if any of those is available

@gabemontero
Copy link
Member Author

can we re-run the travis job if it flakes with an unrelated error?

found https://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit ... seeing if any of those is available

Yeah none of the options there are available. I'm going to refrain from hitting the merge button until can ask the community on Monday, of if someone responds here beforehand.

@adambkaplan
Copy link
Member

@gabemontero I think repo owners can restart the build in Travis. I just did.

@gabemontero
Copy link
Member Author

@gabemontero I think repo owners can restart the build in Travis. I just did.

thanks @adambkaplan

@gabemontero
Copy link
Member Author

docker throttling with latest travis run

docker: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit.

@openshift-merge-robot openshift-merge-robot merged commit 1b68740 into shipwright-io:master Feb 15, 2021
@gabemontero gabemontero deleted the pr-template branch February 15, 2021 13:00
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants