Skip to content

Deduplify content: Remove reviewee content from the review-guidelines #5031

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

Merged
merged 2 commits into from
Aug 18, 2020
Merged

Deduplify content: Remove reviewee content from the review-guidelines #5031

merged 2 commits into from
Aug 18, 2020

Conversation

castrojo
Copy link
Member

@castrojo castrojo commented Aug 12, 2020

reorganize pull request page.

The review-guidelines.md had duplicate content from pull-requests.md. I've removed it so that the review guidelines only have things useful to reviewers and pull-requests.md now has all the tips for getting a good review in one place.

The nitty gritty details of how prow/tide merges PRs is useful, but not so much when you're doing a PR, so I moved it to the bottom of the document so that the PR tips and best practices are listed before them.

And lastly I reformatted pull-requests.md to remove the confusing numerals (they'll look better in the ToC on the contributor site this way and also reformatted it with semantic line feeds, so sorry for the noise there.

Signed-off-by: Jorge O. Castro [email protected]

Which issue(s) this PR fixes:

fixes: #4444

reorganize pull request page.

Signed-off-by: Jorge O. Castro <[email protected]>
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 12, 2020
@k8s-ci-robot k8s-ci-robot added area/contributor-guide Issues or PRs related to the contributor guide sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 12, 2020
@castrojo
Copy link
Member Author

/assign mrbobbytables

Copy link
Member

@mrbobbytables mrbobbytables left a comment

Choose a reason for hiding this comment

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

A few changes and a few ideas, but overall does a good job of deduping 👍

@mrbobbytables mrbobbytables changed the title [WIP] Deduplify content: Remove reviewee content from the review-guidelines Deduplify content: Remove reviewee content from the review-guidelines Aug 17, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2020
@mrbobbytables mrbobbytables added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 17, 2020
@mrbobbytables mrbobbytables added this to the v1.19 milestone Aug 17, 2020
@mrbobbytables
Copy link
Member

/lgtm
/hold
For any other comments 👍
The other parts I specifically called out should be integrated can be done in a separate PR.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 17, 2020
@parispittman
Copy link
Contributor

it would be cool if the reviewer guidelines was turned into a checklist something like

  • does it have tests
  • can this be broken into multiple PRs

etc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: castrojo, markyjackson-taulia

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

@mrbobbytables
Copy link
Member

@parispittman there are a few items we want to add in a follow-up PR, would it be okay if those are added then?

@mrbobbytables
Copy link
Member

discussed, we are good \o/ - changes will follow up in later PR.
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit d27cb26 into kubernetes:master Aug 18, 2020
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. area/contributor-guide Issues or PRs related to the contributor guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracking Issue] Revamping the Contributor Guide
5 participants