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

Clarifying Docs contributor roles #958

Merged
merged 11 commits into from
Mar 14, 2019
Merged

Conversation

samodell
Copy link
Contributor

@samodell samodell commented Mar 5, 2019

No description provided.

@samodell samodell added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2019
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 5, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 5, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

Copy link
Contributor

@carieshmarie carieshmarie left a comment

Choose a reason for hiding this comment

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

Thanks for adding this content! I'm requesting a few changes.

- Has made multiple contributions to the project or community. Contributions may
include, but are not limited to:

- Authoring or reviewing PRs on GitHub in Knative/Docs or Knative/Website
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the repo paths be capitalized? I think of them as lower-case, but I think just because that's how they appear in URLs.

Also, global comment that I think these bullets should have periods at the end of them. Looks like this guidance is applied in some cases, but not in others.


### Requirements

- Reviewer of the Docs repo for at least 3 months or 50% of project lifetime,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the project here Knative itself? I think we're at the 8 month mark (so 50% of project lifetime is 4 mos?), so maybe just hae the at least 3 months requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True!


Once you feel you meet the criteria, you can ask one of the current
approvers to nominate you to become an approver. If all existing
approvers agree that you meet the criteria, and no one objects,
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need the phrase "and no one objects" - feels repetitive.

Suggested change
approvers agree that you meet the criteria, and no one objects,
approvers agree that you meet the criteria,

@samodell
Copy link
Contributor Author

@carieshmarie Addressed your comments; PTAL! :)


- Addresses bugs or issues in their documentation in a timely manner.

### Becoming a member
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to see this section, thanks @samodell , this really helps ;-)

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2019
@knative-prow-robot knative-prow-robot merged commit 3f7a02e into master Mar 14, 2019
@samodell
Copy link
Contributor Author

Oops, @carieshmarie this merged in before you could review my changes. Please let me know if there are any leftover concerns and I'll open a new PR to address.

@gyliu513
Copy link
Contributor

Sorry @samodell and @carieshmarie , I will try to use lgtm in future if we want to extra comments for some PRs. ;-)

@RichieEscarez RichieEscarez deleted the samodell-patch-2 branch March 15, 2019 20:20
vorburger added a commit to vorburger/docs that referenced this pull request Mar 19, 2019
* master:
  fix serving directory path (knative#1015)
  site: insert 'docs' container folder at root (for knative.dev) (knative#1007)
  hack: update ref docs gen script to cleanup tmp dirs (knative#1001)
  hack: configurable repo refs for api docs gen (knative#998)
  Update traffic splitting example to use Release mode (knative#964)
  Update Eventing cronjob-source for Cloudevents (knative#1006)
  Update issue templates -- delete extra template, add security note (knative#1003)
  Update issue templates -- separate feature request from new/changing functionality (knative#1002)
  Clarifying Docs contributor roles (knative#958)
  update OpenShift on install (knative#975)
  Update the monitoring file version (knative#996)
  Update Dockerfile (knative#991)
  update Knative-with-OpenShift from 3.10 to 3.11 (and minor clean up) (knative#974)
  make install/scripts/knative-with-openshift.sh +x and -e (knative#973)
  fix wrong path (knative#989)
  Update README.md (knative#987)
  Change MiniKube installation to require 1.12 cluster. (knative#873)
  fix typo (knative#963)
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.

5 participants