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

Update docs contributor guidelines #1193

Merged
merged 36 commits into from
Apr 19, 2019

Conversation

RichieEscarez
Copy link
Contributor

Add docs PR details:

  • docs
  • code samples
  • branches
  • cherry picks
  • add 0.5 release to the list of releases page

@RichieEscarez RichieEscarez requested a review from rgregg April 17, 2019 18:56
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 17, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2019
@samodell samodell added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs edits labels Apr 17, 2019
@samodell
Copy link
Contributor

I'm planning to do an edit of this later today.

contributing/DOCS-CONTRIBUTING.md Outdated Show resolved Hide resolved
contributing/DOCS-CONTRIBUTING.md Show resolved Hide resolved
contributing/DOCS-CONTRIBUTING.md Outdated Show resolved Hide resolved
contributing/DOCS-CONTRIBUTING.md Show resolved Hide resolved
contributing/DOCS-CONTRIBUTING.md Show resolved Hide resolved
1. Add one or more `cherrypick-#.#` labels to that PR to indicate which
of the past release branches should also be fixed.
1. If you want to complete the fix yourself (best practice), you then open
a subsequent PR by running `git cherry-pick [OriginalPRCommit#]` against
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if the "Original PR commit" reference here makes sense; I wait until the PR is merged and then grab the squashed commit number that shows at the bottom. For example: #1143 shows 3f8fbb2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ive used the language from the template, ptal

contributing/DOCS-CONTRIBUTING.md Outdated Show resolved Hide resolved

## Assigning reviewers

For both documentation and code samples, you should assign your PR to a single
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now, Prow automatically assigns two reviewers from the list of docs approvers and reviewers: https://github.com/knative/docs/blob/master/OWNERS_ALIASES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prow is nice and mostly reliable but the goal here is to address the lack of SME eyes on certain items, particularly code sample related items. Instead of maintainers manually assigning items (especially when they are from peers) it would expedite a review if the author assigned a reviewer immediately.

Copy link
Contributor

@samodell samodell Apr 18, 2019

Choose a reason for hiding this comment

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

Oh, my point was that you said explicitly to only assign it to one person, and I was just pointing out that Prow automatically assigns two, so a person could see the auto-assignments as be like, "Oops, I need it to only be assigned to one person."

But also there's some restrictions on who is able to assign issues/PRs -- I think you have to be a member of the Knative org, so some contributors (esp. first timers) might not be able to assign anyone themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, good point. others can use /assign flag with prow, will add that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ptal

contributing/DOCS-CONTRIBUTING.md Outdated Show resolved Hide resolved
contributing/DOCS-CONTRIBUTING.md Outdated Show resolved Hide resolved
@samodell samodell removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs edits labels Apr 17, 2019
For a list of the available branches in the `knative/docs` repo, see
[Documentation Releases](https://github.com/knative/docs/blob/master/doc-releases.md).

## Assigning reviewers
Copy link
Contributor

Choose a reason for hiding this comment

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

GitHub has "Reviewers" as well as "Assignees". Should we differentiate between the two?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry, this comment was left as "pending" and didn't get batched in with my first review somehow...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i agree, will add this too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ptal

@RichieEscarez
Copy link
Contributor Author

Thanks for all the feedback (and the fixes)! This is ready for a second pass, PTAL

@samodell
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RichieEscarez, samodell

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 [RichieEscarez,samodell]

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

@knative-prow-robot knative-prow-robot merged commit 39e9017 into knative:master Apr 19, 2019
@RichieEscarez RichieEscarez deleted the contributing branch May 2, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. 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.

6 participants