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

access: Gerrit Approver Access ("approvers") #24964

Closed
namusyaka opened this issue Apr 20, 2018 · 2 comments
Closed

access: Gerrit Approver Access ("approvers") #24964

namusyaka opened this issue Apr 20, 2018 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@namusyaka
Copy link
Member

namusyaka commented Apr 20, 2018

See https://golang.org/wiki/GerritAccess

Want: https://go-review.googlesource.com/#/admin/groups/1005,members

My gerrit name: Kunpei Sakai
My gerrit email: namusyaka@gmail.com

All my gardening issues: https://github.com/golang/go/issues?utf8=%E2%9C%93&q=is%3Aissue+commenter%3Anamusyaka+
All my participating CLs: https://go-review.googlesource.com/q/namusyaka%2540gmail.com


At first, @spf13 thank you for showing clear policy at #24850 . I was just thinking about requesting an approver in gerrit for maintenance of the x/net/html package. Of course I am going to continue committing about other packages such as cmd/compile and net/http.

I have improved the parser implementation of the x/net/html package based on the implementation of chromium (blink) and webkit for a while, but I thought that we need to tackle this in the medium to long term.
If necessary, we will need to develop it while communicating with the chromium team. I'm going to do that.

/cc @andybons @bradfitz @nigeltao

@nigeltao
Copy link
Contributor

Disclaimer: I don't have experience with @namusyaka's contributions outside of x/net/html.

As for x/net/html, please correct me if https://go-review.googlesource.com/q/owner:namusyaka%2540gmail.com+project:net+html is missing any of your contributions.

How will gaining the approver bit change things, in practice? Code changes (in the Go main repo and under x) still need to be reviewed before being committed. Currently, those reviewers (i.e. me for substantial changes, @bradfitz and maybe others for small changes or rollbacks) can approve and submit as soon as they're happy from a code review perspective. Conversely, even if you had the approver bit, you'd still have to wait for a code reviewer to sign off.

Also, the approval bit generally implies familiarity with the code they're approving for. On the other hand, https://go-review.googlesource.com/c/net/+/94838 is a recent change of yours, which had lots of review comments. I'm not saying that you're a bad programmer. Even good programmers miss some things when modifying unfamiliar code. Not every nuance is captured in the code itself, its nearby comments and the test suite, and sometimes you just need to know what far-away documentation is relevant (or missing!). I'm just saying that you're not yet completely familiar with the subtleties of the x/net/html package. That familiarity will surely grow over time, the more you work with it, but I don't think you're there yet, now.

Sorry to sound negative. I appreciate the work you're doing in x/net/html, and I would like to see it continue. I'm just concious of the risk, in terms of operational security or otherwise, of handing out approver bits, and we don't have the technical capability to limit the approval bit to subsets of the repos, e.g. "just x/net/html".

I'll repeat that I'm not familiar with your other work, such as in cmd/compile and net/http, which may very well be in favor of us granting you the approval bit. I'm only just one opinion on this issue, specifically for you and generally in terms of policy, and am happy for other people to have different opinions.

@namusyaka
Copy link
Member Author

namusyaka commented Apr 22, 2018

@nigeltao I appreciate your comment.

Please let me supplement some.

How will gaining the approver bit change things, in practice?

Currently, the parser of the x/net/html package contains a lot of code (after reviewing, of course) that I implemented. On the contrary, it was a statement that I am willing to sustainably maintain.
However, as you say, I am not a familiar with all of the x/net/html package.

And I have been doing continuous contributions (probably go 20+ commits + other packages => 40+ commits) from last year on other packages and gardening some issues, but it is hard to say that I am perfectly familiar with cmd/compile or net/http.
Even if I get the bit, I understand that reviewing my own patch is obviously essential.
If there is one merit, it will be able to smoothly perform bug fixes from others on the code that I created.

I don't know the clear criteria to get the bit (or unexistence?).
But anyway, I will continue to commit to go and its subpackages.

Thanks.

@bcmills bcmills added this to the Unreleased milestone Apr 27, 2018
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 27, 2018
@golang golang locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants