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

adding n3wscott to approvers. #465

Merged
merged 2 commits into from
Mar 19, 2019

Conversation

n3wscott
Copy link
Contributor

@n3wscott n3wscott commented Sep 25, 2018

Updated: March 18

From [ROLES.md]
(https://github.com/knative/docs/blob/1b52510c44168a99efe6179c523cd68bdac1e07b/community/ROLES.md):

Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter

Earliest review: 2018-07-02, 259 days ago
https://github.com/knative/eventing/pulls?utf8=%E2%9C%93&q=is%3Apr+reviewed-by%3An3wscott+sort%3Acreated-asc

Primary reviewer for at least 10 substantial PRs to the codebase

Reviewed 29 L, XL, or XXL PRs

10 substantial examples:

  1. Broker, Trigger, and Namespace Controllers #788 - 20+ comments

  2. [WIP] Add NATS Streaming as a new bus implementation #491 - 15 comments

  3. Defaultcontroller #843 - 12 comments

  4. Create the in-memory-channel ClusterProvisioner #484 - 25+ comments

  5. Adds Kafka Channel Provisioner Controllers #468 - 20+ comments

original submissions:

  1. Add a new Channel CRD to the eventing group #430 - Majority reviewed.

  2. Bus monitor polish #382 - Majority reviewed, this is a solid review.

  3. add AWS SQS event source #328 - This is still in flight and has not been merged.

  4. Enforce all code dirs have tests #453 - This is not yet merged (and may not be mergeable).

  5. add support for Subsciption.replyTo property #325 - This was a split review, but I'd give credit.

Reviewed or merged at least 30 PRs to the codebase

Reviewed or merged 84 PRs
https://github.com/knative/eventing/pulls?utf8=%E2%9C%93&q=is%3Apr+reviewed-by%3An3wscott

Nominated by an area lead with no objections from other leads

Current eventing lead is @vaikas-google.

/cc @evankanderson @vaikas-google @grantr @scothis

--

Past PRs:

  1. Empty body stops ReplyTo. #467 - 7 comments
  2. Generate unique, stable pubsub topic and subscription IDs #458 - 8 comments

Original 10:

  1. Subscription Controller basics. #437
  2. Add a new Channel CRD to the eventing group #430
  3. Create new Subscription in pkg/apis/eventing as per the new spec. #421
  4. Bus monitor polish #382
  5. add AWS SQS event source #328
  6. Run all controllers in the same process #313
  7. Cluster scoped Bus #117
  8. Enforce all code dirs have tests #453
  9. add support for Subsciption.replyTo property #325
  10. Add configmap for flow controller #324

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 25, 2018
@grantr
Copy link
Contributor

grantr commented Sep 26, 2018

👍 Scott's the top contributor by number of PRs merged. He's also made supporting contributions to https://github.com/knative/pkg that aren't counted here.

@evankanderson
Copy link
Member

I took a look at the top 10 PRs. I think the reviews are a bit light, so I'm going to object at this time:

10 substantial examples:

#437

The majority of reviews on this PR came from grantr and mattmoor.

#430

Majority reviewed.

#421

This had a large number of reviewers, but I wouldn't consider Scott's a majority review.

#382

Majority reviewed, this is a solid review.

#328

This is still in flight and has not been merged.

#313

Small review (or @grantr produces very nice code...)

#117

This was majority reviewed by ericbottard and pmorie

#453

This is not yet merged (and may not be mergeable).

#325

This was a split review, but I'd give credit.

#324

This was majority reviewed by grantr.

@evankanderson
Copy link
Member

To be clear, I'm just trying to apply our rules evenly -- and there's certainly space for strong contributors who mostly produce rather than review code (which would be the measure of "PRs merged"). I'm also happy to revisit this decision in a week or two if Scott starts being a code review monster. 🥇

@n3wscott
Copy link
Contributor Author

/open

@n3wscott
Copy link
Contributor Author

/reopen

@knative-prow-robot
Copy link
Contributor

@n3wscott: Reopening this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@scothis
Copy link
Contributor

scothis commented Oct 26, 2018

@n3wscott can you add a comment about why you reopened this PR and summarize what has changed besides being "a week or two" later?

@n3wscott
Copy link
Contributor Author

I had updated the description and added updated

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 26, 2018
@evankanderson
Copy link
Member

It looks like 5 of the 10 review candidates haven't merged yet -- I agree that @n3wscott is prolific in writing code, but writing and reviewing are different skills. I think it's fine to have active contributors who aren't approvers, and I'm sure that Scott will end up reviewing more code.

@n3wscott
Copy link
Contributor Author

Will reopen when those are submitted.

@n3wscott n3wscott closed this Oct 29, 2018
@n3wscott n3wscott reopened this Mar 18, 2019
@n3wscott
Copy link
Contributor Author

I'm back!

@evankanderson
Copy link
Member

This looks like a solid collection of reviews. #843 in particular has some detailed feedback alongside @vaikas-google 's feedback. #484 also has some good feedback, particularly around managing Conditions.

- inlined
- n3wscott
- scothis
- Harwayne
Copy link
Contributor

Choose a reason for hiding this comment

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

this diff looks odd because it looks like you're adding Harwayne (who's already in the head as well as removing inlined and scothis)

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what view I was looking at...

@vaikas
Copy link
Contributor

vaikas commented Mar 19, 2019

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, vaikas-google

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2019
@knative-prow-robot knative-prow-robot merged commit 7c13060 into knative:master Mar 19, 2019
mgencur pushed a commit to mgencur/eventing that referenced this pull request Dec 14, 2023
🤖 Triggering CI on branch 'release-next' after synching to upstream/main
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 Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants