upcoming code review requirement changes
3,269 views
Skip to first unread message
Russ Cox
Apr 4, 2022, 9:00:14 AM4/4/22
to golang-dev
Hi all,
For compliance and supply chain security reasons, Google recently revisited the code review requirements we use in all settings, both internal development and open source. We are now required to have two Google employees review each change before it is shipped to users, which for most of our tools means submitted in Gerrit (tools like “go get” and “gotip download” and the go.dev auto-deployment read directly from Gerrit).
As a result, later this month we will be adding a new Gerrit submit requirement that two Google employees have either uploaded or contributed a positive Code-Review vote (+1 or +2) to each change before it can be submitted. This will replace the current Trust+1 scheme.
In x/website repo, Code-Review+2 and Trust+1 votes have been restricted to Google employees on the Go team for the past half year or so, for related supply chain security reasons (changes to the web site are automatically deployed and could potentially redirect downloads of the Go distribution). That repo will be the first to adopt the new system, and once we are confident it is working well there, we will move on to the other repos. I will send a reply to this thread and update go.dev/wiki/GerritAccess when all repos have been converted to the new system.
The x/proposal and x/scratch repos will continue to allow single-person self-review, since they do not contain any code being shipped to users.
Everyone who can currently Code-Review+2 a CL will still be able to do that, and x/website will reopen to Code-Review+2 from all our approvers. Some CLs will then need additional Code-Review+1 votes from Google employees. I realize this change may be disappointing, and I apologize. All of us on the Go team at Google remain very committed to Go being open source and to preserving our open development processes. Once the new system is in place, I will share a link to a dashboard that we will be checking regularly for CLs that have a Code-Review+2 but need additional Code-Review+1 votes.
Thank you for all your contributions and help working on Go. We couldn't do it without all of you.
Best,
Russ
For compliance and supply chain security reasons, Google recently revisited the code review requirements we use in all settings, both internal development and open source. We are now required to have two Google employees review each change before it is shipped to users, which for most of our tools means submitted in Gerrit (tools like “go get” and “gotip download” and the go.dev auto-deployment read directly from Gerrit).
As a result, later this month we will be adding a new Gerrit submit requirement that two Google employees have either uploaded or contributed a positive Code-Review vote (+1 or +2) to each change before it can be submitted. This will replace the current Trust+1 scheme.
In x/website repo, Code-Review+2 and Trust+1 votes have been restricted to Google employees on the Go team for the past half year or so, for related supply chain security reasons (changes to the web site are automatically deployed and could potentially redirect downloads of the Go distribution). That repo will be the first to adopt the new system, and once we are confident it is working well there, we will move on to the other repos. I will send a reply to this thread and update go.dev/wiki/GerritAccess when all repos have been converted to the new system.
The x/proposal and x/scratch repos will continue to allow single-person self-review, since they do not contain any code being shipped to users.
Everyone who can currently Code-Review+2 a CL will still be able to do that, and x/website will reopen to Code-Review+2 from all our approvers. Some CLs will then need additional Code-Review+1 votes from Google employees. I realize this change may be disappointing, and I apologize. All of us on the Go team at Google remain very committed to Go being open source and to preserving our open development processes. Once the new system is in place, I will share a link to a dashboard that we will be checking regularly for CLs that have a Code-Review+2 but need additional Code-Review+1 votes.
Thank you for all your contributions and help working on Go. We couldn't do it without all of you.
Best,
Russ
U Cirello
Apr 4, 2022, 9:05:27 AM4/4/22
to Russ Cox, golang-dev
Hi,
If I am correctly informed, within Google, every change to the main repository has to be reviews by another class of reviewers whose job is to confirm readability. For the sake of transparency, wouldn't it be better to actually establish a "Compliance+1" vote with assigned people to the job. So to make it transparent when an approval happened because of the change vs an approval that happened because of compliance.
My understanding is that by enforcing two "vanilla" reviewers, each reviewer may not understand what role they are playing in the approval.
Thanks,
Ulderico
--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAA8EjDQv2CTZPmiMHM4HWJnT3QTd630%3DrB-dPeDDjQj6%3DhyoqQ%40mail.gmail.com.
Dominik Honnef
Apr 4, 2022, 9:38:34 AM4/4/22
to golang-dev
If two Googlers need to sign off on CLs then what is the point of non-Googlers being able to +2 changes? Even if I +2 something, 1-2 Googlers still need to review the change and at least leave a +1. But since their +1 signs off on my +2, it is effectively a +2, too. Plus their review probably needs to be as in-depth as an actual +2, anyway, to prevent those pesky supply chain attacks. So whether I +2 a CL or not makes no difference. Am I missing something?
Alberto Donizetti
Apr 4, 2022, 9:54:30 AM4/4/22
to Dominik Honnef, golang-dev
> Am I missing something?
I don't think you're missing anything. This change effectively restricts
the committer group to Google employees.
Alberto
I don't think you're missing anything. This change effectively restricts
the committer group to Google employees.
Alberto
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/8e9a6611-240d-49a0-bb75-270837459b7fn%40googlegroups.com.
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
Russ Cox
Apr 4, 2022, 9:56:15 AM4/4/22
to Dominik Honnef, golang-dev
On Mon, Apr 4, 2022 at 9:38 AM Dominik Honnef <dominik...@gmail.com> wrote:
If two Googlers need to sign off on CLs then what is the point of non-Googlers being able to +2 changes? Even if I +2 something, 1-2 Googlers still need to review the change and at least leave a +1. But since their +1 signs off on my +2, it is effectively a +2, too. Plus their review probably needs to be as in-depth as an actual +2, anyway, to prevent those pesky supply chain attacks. So whether I +2 a CL or not makes no difference. Am I missing something?
I don't think that's quite true. Two shallow +1 reviews is not the same as one in-depth +2 review. This is the text I plan to put on the GerritAccess page once we switch to the new system:
Every CL requires both a code review (Code-Review+2) from an approver and the involvement of two Google employees, either as code uploader or as a reviewer voting at least Code-Review+1. Requiring multiple people ensures that code cannot be submitted unilaterally from a single compromised account. Once a review has a Code-Review+2 and the necessary Google involvement, it can be submitted by any approver. All these rules are enforced by the Gerrit server.
A Code-Review+2 vote means that you have read the change and are confident that it is correct and appropriate to submit. Typically, you should only Code-Review+2 code in directories or packages that you "own"; the exception is trivial and obviously correct changes. Note that all user-visible new features or changes—new API, new command-line flags, and so on—need to go through the proposal process. The CLs should reference the specific accepted proposal in the commit message (“For #NNN.”).
A Code-Review+1 vote means that you have read the change and believe it seems reasonable but aren’t making the definitive judgement that Code-Review+2 indicates. It also means that are confident that the change does not introduce any sort of security vulnerability or other clearly inappropriate code change.
We fully expect that CLs will continue to land with only non-Googler Code-Review+2 reviews as they do today.
Best,
Russ
Russ Cox
Apr 4, 2022, 10:04:02 AM4/4/22
to U Cirello, golang-dev
On Mon, Apr 4, 2022 at 9:05 AM U Cirello <ulderi...@gmail.com> wrote:
If I am correctly informed, within Google, every change to the main repository has to be reviews by another class of reviewers whose job is to confirm readability. For the sake of transparency, wouldn't it be better to actually establish a "Compliance+1" vote with assigned people to the job. So to make it transparent when an approval happened because of the change vs an approval that happened because of compliance.
For what it's worth, we did look into this, but the system that has been developed in Gerrit does not work this way. And this is more like a code review than readability is, so I think it will work out okay. But I understand your point. In my previous reply on this thread I quoted the description that will go on the GerritAccess wiki page, in case that helps clarify matters. Thanks.
Best,
Russ
Dan Kortschak
Apr 4, 2022, 4:01:09 PM4/4/22
to golan...@googlegroups.com
now see review length blow out the the square factor for reviews that
already can take a long time (I have a tiny change that is nearly a
year old).
Dan
Russ Cox
Apr 4, 2022, 5:40:36 PM4/4/22
to Dan Kortschak, golan...@googlegroups.com
On Mon, Apr 4, 2022 at 4:01 PM 'Dan Kortschak' via golang-dev <golan...@googlegroups.com> wrote:
I can understand the rationale for this, but I'm concerned that we will
now see review length blow out the the square factor for reviews that
already can take a long time (I have a tiny change that is nearly a
year old).
Hi Dan,
I understand your concern. It should not cause a quadratic effect, though.
The hard part is getting the Code-Review+2, which isn't changing at all.
Once that is done, the CL will show up on a dashboard that we'll be watching
and we'll add the necessary Code-Review+1 votes after a quick review for
security and related problems, not an in-depth +2-level review.
Best,
Russ
Daniel Martí
Apr 4, 2022, 8:35:47 PM4/4/22
to Russ Cox, golang-dev
Hello Russ,
Thanks for being upfront about this change. I expected to lose the
ability to click "submit" sooner than later due to the submit queue bot,
though I admit I didn't expect a wider change like this one.
Before I get to my concerns, I have some questions about GerritAccess:
> Every CL requires both a code review (Code-Review+2) from an approver and
> the involvement of two Google employees, either as code uploader or as a
> reviewer voting at least Code-Review+1. Requiring multiple people ensures
> that code cannot be submitted unilaterally from a single compromised
> account.
If you want to protect the project against a single approver account
being compromised, it seems to me like the existing system with two
Trust votes already accomplishes that. I assume this wording would need
to be adjusted, as it describes the current system.
Second, I would like to understand what it is about Google that prevents
two of their approver employees from being compromised at once.
There's clearly something that external approvers are missing, as
otherwise this wouldn't be a factor in the equation at all.
For example, I would be completely open to Go approvers being required
to have strong multi-factor authentication, even for each Gerrit session
where they approve or submit changes. This kind of security enhancement
hasn't been touched upon before, as far as I can tell.
Onto the concerns: your prediction is right that I am disappointed.
I'll try to explain why in three main points below:
First, it feels like the set of Go approvers loses much of its meaning.
Google company-wide security policies aside, it's as if the project no
longer trusted external contributors to perform their duties.
If they can't be trusted to keep their accounts reasonably secure,
should they be trusted to maintain parts of the Go project?
The above also goes the other way: as an external contributor and
maintainer, should I really invest that much time and energy in the
project if the project doesn't reciprocate with trust?
I understand that the "Googlers only" rule is probably not your choice,
but its effect on the project and community is the same regardless.
Second, this change will make timezone differences more painful.
Practically all Go team members at Google are in US timezones,
meaning that people like me have little overlap in working hours.
Our work is generally asynchronous and that is fine, but being able to
submit a CL without waiting hours or days is still valuable.
For example, if a "long" builder in build.golang.org unexpectedly breaks
due to a CL of mine, I can notice that and attempt to fix it if I'm
around, but not if the CL gets merged six hours later when I'm AFK.
I expect this will be the reality, because the new Gerrit dashboard
won't be manned anywhere close to 24/7.
Third, no matter how well engineered the Gerrit dashboard is,
it will still ultimately lead to more work for the Go team at Google,
which is worrying given the project is already lagging behind in reviews.
I'm too far removed to know who is responsible for the supply chain
security measures, but I would assume they can realise that they are not
viable unless the Go team magically doubles in headcount.
In summary, Go needs more maintainers and reviewers, and doing that work
outside of Google has always been a delicate balance. There are calls
and emails that I'll never be included in due to not being at Google,
and I've come to accept that. However, I do fear that this change will
make it even less likely to actively participate in Go from the outside.
Thanks for being upfront about this change. I expected to lose the
ability to click "submit" sooner than later due to the submit queue bot,
though I admit I didn't expect a wider change like this one.
Before I get to my concerns, I have some questions about GerritAccess:
> Every CL requires both a code review (Code-Review+2) from an approver and
> the involvement of two Google employees, either as code uploader or as a
> reviewer voting at least Code-Review+1. Requiring multiple people ensures
> that code cannot be submitted unilaterally from a single compromised
> account.
being compromised, it seems to me like the existing system with two
Trust votes already accomplishes that. I assume this wording would need
to be adjusted, as it describes the current system.
Second, I would like to understand what it is about Google that prevents
two of their approver employees from being compromised at once.
There's clearly something that external approvers are missing, as
otherwise this wouldn't be a factor in the equation at all.
For example, I would be completely open to Go approvers being required
to have strong multi-factor authentication, even for each Gerrit session
where they approve or submit changes. This kind of security enhancement
hasn't been touched upon before, as far as I can tell.
Onto the concerns: your prediction is right that I am disappointed.
I'll try to explain why in three main points below:
First, it feels like the set of Go approvers loses much of its meaning.
Google company-wide security policies aside, it's as if the project no
longer trusted external contributors to perform their duties.
If they can't be trusted to keep their accounts reasonably secure,
should they be trusted to maintain parts of the Go project?
The above also goes the other way: as an external contributor and
maintainer, should I really invest that much time and energy in the
project if the project doesn't reciprocate with trust?
I understand that the "Googlers only" rule is probably not your choice,
but its effect on the project and community is the same regardless.
Second, this change will make timezone differences more painful.
Practically all Go team members at Google are in US timezones,
meaning that people like me have little overlap in working hours.
Our work is generally asynchronous and that is fine, but being able to
submit a CL without waiting hours or days is still valuable.
For example, if a "long" builder in build.golang.org unexpectedly breaks
due to a CL of mine, I can notice that and attempt to fix it if I'm
around, but not if the CL gets merged six hours later when I'm AFK.
I expect this will be the reality, because the new Gerrit dashboard
won't be manned anywhere close to 24/7.
Third, no matter how well engineered the Gerrit dashboard is,
it will still ultimately lead to more work for the Go team at Google,
which is worrying given the project is already lagging behind in reviews.
I'm too far removed to know who is responsible for the supply chain
security measures, but I would assume they can realise that they are not
viable unless the Go team magically doubles in headcount.
In summary, Go needs more maintainers and reviewers, and doing that work
outside of Google has always been a delicate balance. There are calls
and emails that I'll never be included in due to not being at Google,
and I've come to accept that. However, I do fear that this change will
make it even less likely to actively participate in Go from the outside.
Russ Cox
Apr 4, 2022, 10:00:18 PM4/4/22
to Daniel Martí, golang-dev
Hi Daniel,
The existing system with two Trust+1 votes does protect against single-account compromises. The issue is unfortunately not whether we on the Go team at Google trust you or any of the other approvers - we certainly do. The fact that it's no longer about trust is part of why we are dropping the Trust label as the enforcement mechanism.
On the whole, you are right and I agree with you.
Unfortunately, as I noted briefly in the first message, Google also has compliance reasons - independent of software supply chain concerns - for the rule about review by two Google employees, and I can't say more about the details. All I can say is that we are now required to do this review before a change to Go ships to users, and our many tools that read directly from the latest commits in the Git repos force our hand as far as the review happening before code is submitted to those repos.
The existing system with two Trust+1 votes does protect against single-account compromises. The issue is unfortunately not whether we on the Go team at Google trust you or any of the other approvers - we certainly do. The fact that it's no longer about trust is part of why we are dropping the Trust label as the enforcement mechanism.
I understand what you are saying about it feeling like being a Go approver loses its meaning. We intend for that not to be true in practice, as we still intend to separate Code-Review+2 from the compliance-induced Code-Review+1. But again I understand what you are saying, and it is absolutely on us to demonstrate that being a Go approver still means something as we shift to the new system. The same applies to timezone lag as well as creating more work for us on the Go team at Google. We know about those problems and are going to work to ameliorate them. Again it's on us to make that reality once the new system is in place.
For the past six months, I have been trying to find some way, any way, to avoid writing today's email. Obviously I was unsuccessful in that endeavor, and for that I am truly sorry and I apologize. Having failed at not making the change at all, we are committed to doing everything we can to make the change as unobtrusive as possible.
I wish I had a better answer. Again, my apologies.
Best,
Russ
Daniel Martí
Apr 5, 2022, 7:05:45 PM4/5/22
to Russ Cox, golang-dev
Hello Russ,
I appreciate your frankness, and I can see you anticipated the reaction
to the announcement and tried to prevent it altogether.
I seem to understand that this change is not really about security,
then; rather about some opaque set of compliance rules at Google
which are now being imposed on open source projects like Go.
I understand that you can't say more about that. I also can't help but
wonder who is in charge of this top-down decision. To anyone who
understands how open source projects and communities work, it honestly
feels like a rather short-sighted move. If the concern was simply
security, I am sure that we could figure out a better compromise.
I can see you have some plans to reduce the harm done by this change,
and I thank you for the effort, though I fear the end result will be
perceived as practically the same - a big part being the optics.
Put another way, we already apologize to contributors for requiring them
to have a Google account and to sign the Google CLA [1]. It seems like
we'll also be apologizing for requiring all approvals to go through
Google employees. We can collectively feel sorry about the situation,
but that doesn't sound like a good long-term plan for the OSS project :)
[1]: https://github.com/golang/go/issues/49383
I appreciate your frankness, and I can see you anticipated the reaction
to the announcement and tried to prevent it altogether.
I seem to understand that this change is not really about security,
then; rather about some opaque set of compliance rules at Google
which are now being imposed on open source projects like Go.
I understand that you can't say more about that. I also can't help but
wonder who is in charge of this top-down decision. To anyone who
understands how open source projects and communities work, it honestly
feels like a rather short-sighted move. If the concern was simply
security, I am sure that we could figure out a better compromise.
I can see you have some plans to reduce the harm done by this change,
and I thank you for the effort, though I fear the end result will be
perceived as practically the same - a big part being the optics.
Put another way, we already apologize to contributors for requiring them
to have a Google account and to sign the Google CLA [1]. It seems like
we'll also be apologizing for requiring all approvals to go through
Google employees. We can collectively feel sorry about the situation,
but that doesn't sound like a good long-term plan for the OSS project :)
[1]: https://github.com/golang/go/issues/49383
Russ Cox
Apr 6, 2022, 4:30:31 PM4/6/22
to golang-dev
Hi all,
Status update: We've made the change in x/website. The dashboard for CLs that need more Google reviews is https://go.dev/s/needs-review. We may adjust the search over time, so if you bookmark, bookmark that link and not the URL it currently redirects to. Probably next week we will make the change in the other repos.
Thanks and apologies again.
Best,
Russ
Benny Siegert
Apr 7, 2022, 5:13:38 AM4/7/22
to Russ Cox, golang-dev
It does not look like this redirects to a valid URL:
<a href="is:open label:Code-Review=+2 label:Review-Enforcement=NEED
label:Trybot-Result=+1">Found</a>.
--
Benny
--
Benny
<a href="is:open label:Code-Review=+2 label:Review-Enforcement=NEED
label:Trybot-Result=+1">Found</a>.
--
Benny
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAA8EjDS%2BcxyuByRhRmmRoX6x6psbbRR2yXdnmEFermY804vmiQ%40mail.gmail.com.
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
--
Benny
Dmitri Shuralyov
Apr 7, 2022, 11:12:33 AM4/7/22
to golang-dev
Thanks Benny, the redirect URL is corrected.
Benny Siegert
Apr 7, 2022, 12:05:17 PM4/7/22
to Dmitri Shuralyov, golang-dev
Works for me, thanks!
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/4dc17c1c-6e2f-41c0-ba32-d49af784f5can%40googlegroups.com.
--
Benny
--
Benny
Russ Cox
Apr 11, 2022, 12:30:04 PM4/11/22
to golang-dev
Hi all,
Status update: We've made the change in all the repos, except proposal and scratch.
https://go.dev/wiki/GerritAccess has been updated to describe the state of the world.
https://go.dev/s/needs-review shows the CLs that are waiting for more Google reviews.
Let us know if you run into any unexpected problems.
Thanks!
Best,
Russ
Russ Cox
Apr 11, 2022, 12:31:05 PM4/11/22
to golang-dev
One more note: If you had configured "mail = codereview mail -trust" in your $HOME/.gitconfig,
then you will need to remove the -trust flag, since the Trust label no longer exists.
Best,
Russ
Brad Fitzpatrick
Apr 21, 2022, 6:37:53 PM4/21/22
to Russ Cox, golang-dev
On Mon, Apr 11, 2022 at 9:30 AM Russ Cox <r...@golang.org> wrote:
Hi all,
Status update: We've made the change in all the repos, except proposal and scratch.https://go.dev/wiki/GerritAccess has been updated to describe the state of the world.https://go.dev/s/needs-review shows the CLs that are waiting for more Google reviews.Let us know if you run into any unexpected problems.
There are a bunch queued up in that needs-review link, including one of mine I'm getting impatient about.
How often do Googlers check those?
GTAL (Googlers, Take A Look)
Benny Siegert
Apr 22, 2022, 3:12:49 AM4/22/22
to Brad Fitzpatrick, Russ Cox, golang-dev
Thanks for the reminder, Brad. I just went through the list (and Than
did too, apparently). All but one are unblocked now.
--
Benny
--
Benny
did too, apparently). All but one are unblocked now.
--
Benny
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAFzRk032bhzgSKRREfNp2iVnH94dKe8qbKfFmwZOSksjc%3Dx1-w%40mail.gmail.com.
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
--
Benny
brainman
Jul 1, 2022, 6:53:52 PM7/1/22
to golang-dev
Hello Googlers with +1 access.
Please +1 https://go-review.googlesource.com/c/sys/+/413495 CL.
Thank you.
Alex
brainman
Jul 1, 2022, 9:51:13 PM7/1/22
to golang-dev
brainman
Jul 31, 2022, 2:10:43 AM7/31/22
to golang-dev
I need +1 from Googler to submit https://go-review.googlesource.com/c/sys/+/419394
Than already reviewed the CL. I need another reviewer.
Thank you.
Alex
brainman
Aug 19, 2022, 5:46:23 AM8/19/22
to golang-dev
I need single +1 from Googler to submit https://go-review.googlesource.com/c/go/+/416975
Thank you.
Alex
brainman
Aug 19, 2022, 8:50:13 PM8/19/22
to golang-dev
brainman
Sep 13, 2022, 5:18:45 AM9/13/22
to golang-dev
brainman
Sep 17, 2022, 9:07:48 PM9/17/22
to golang-dev
David Chase
Sep 17, 2022, 9:11:17 PM9/17/22
to brainman, golang-dev
done
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/6a7f5c2a-841a-4667-af74-ee6aff05ff15n%40googlegroups.com.
brainman
Sep 19, 2022, 4:28:07 AM9/19/22
to golang-dev
Benny Siegert
Sep 19, 2022, 4:53:57 AM9/19/22
to brainman, golang-dev
Done!
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/bb8b7637-646d-4062-9f95-d25cd7877f06n%40googlegroups.com.
--
Benny
--
Benny
lab...@linux.vnet.ibm.com
Oct 7, 2022, 9:30:34 AM10/7/22
to golang-dev
I need one Googler +1 on https://go-review.googlesource.com/c/go/+/427634
woolf ian
Oct 7, 2022, 7:30:19 PM10/7/22
to golang-dev
Ian Lance Taylor
Oct 7, 2022, 10:31:21 PM10/7/22
to golang-dev
In general it should not be necessary to ask for an additional Googler
review. If your CL shows up on https://go.dev/s/needs-review, then
some Googler will get to it soon.
If your CL does not show up on that list, then most likely it has an
unresolved comment, or the trybots have not been run.
Please do ping if your CL is not on that list and you don't understand
why, or if it has been on the list for several days. But in general
pinging should not be required.
Thanks,
Ian
review. If your CL shows up on https://go.dev/s/needs-review, then
some Googler will get to it soon.
If your CL does not show up on that list, then most likely it has an
unresolved comment, or the trybots have not been run.
Please do ping if your CL is not on that list and you don't understand
why, or if it has been on the list for several days. But in general
pinging should not be required.
Thanks,
Ian
Message has been deleted
Ian Lance Taylor
Oct 10, 2022, 5:22:19 PM10/10/22
to lab...@linux.vnet.ibm.com, golang-dev
On Mon, Oct 10, 2022 at 8:14 AM lab...@linux.vnet.ibm.com
<lab...@linux.vnet.ibm.com> wrote:
>
> I had a change on the needs-review list for 3 weeks so that is why I posted something here. When you say 'ping' do you mean this post in this mailing list or somewhere else like the CL?
OK, that's bad. Sorry about that. What is the CL number?
If a CL on the needs-review list is not getting reviewed, then
probably the most effective ping will be to write to golang-dev.
Ian
<lab...@linux.vnet.ibm.com> wrote:
>
> I had a change on the needs-review list for 3 weeks so that is why I posted something here. When you say 'ping' do you mean this post in this mailing list or somewhere else like the CL?
OK, that's bad. Sorry about that. What is the CL number?
If a CL on the needs-review list is not getting reviewed, then
probably the most effective ping will be to write to golang-dev.
Ian
> --
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/e94e071d-8119-43eb-b59c-7cf6dcc3f534n%40googlegroups.com.
> You received this message because you are subscribed to the Google Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
Gerardo Bustani
Oct 11, 2022, 8:22:29 AM10/11/22
to golang-dev
Hi good afternoon golang dev's, I'm starting in golang blogs and I don't where to post this kind of questions,
Learning fiber framework and JWT Auth I'm getting in the register Func and Login Func the user Id is saving correctly on DB. The cookie and JWT are getting correctly and disply the cookie and preseve on the frontEnd, but when I want to get the login UserId on Controller I got 0 did some one know's what is happening?
I leave the code hopping have some orientation.
```
// Midleware:
const SecretKey = "secret"
func IsAuthenticated(c *fiber.Ctx) error {
cookie := c.Cookies("jwt")
token, err := jwt.ParseWithClaims(cookie, &jwt.RegisteredClaims{}, func(token *jwt.Token)
(interface{}, error) {
return []byte(SecretKey), nil
})
if err != nil || !token.Valid {
c.Status(fiber.StatusUnauthorized)
return c.JSON(fiber.Map{
"message": "unauthenticated",
})
}
return c.Next()
}
func GetUserId(c *fiber.Ctx) (uint, error) {
cookie := c.Cookies("jwt")
log.Println("Cookie .........: ", cookie)
token, err := jwt.ParseWithClaims(cookie, &jwt.RegisteredClaims{}, func(token *jwt.Token)
(interface{}, error) {
return []byte(SecretKey), nil
})
log.Println("Token .........: ", token)
log.Println("Error .........: ", err)
if err != nil {
return 0, err
}
// var user dto.User
// expireTime := time.Now().Add(24 * time.Hour)
// payloads := jwt.RegisteredClaims{
// Subject: strconv.Itoa(int(user.Id)),
// ExpiresAt: &jwt.NumericDate{Time: expireTime},
// }
payload := token.Claims.(*jwt.RegisteredClaims)
id, _ := strconv.Atoi(payload.Subject)
return uint(id), nil
}
func GenerateJWT(id uint) (string, error) {
expireTime := time.Now().Add(24 * time.Hour)
var user dto.User
token, err := jwt.NewWithClaims(jwt.SigningMethodHS256,
jwt.RegisteredClaims{
Subject: strconv.Itoa(int(user.Id)),
ExpiresAt: &jwt.NumericDate{Time: expireTime},
}).SignedString([]byte(SecretKey))
if err != nil {
log.Println(err)
}
return token, err
}
//Controller:
func User(c *fiber.Ctx) error {
var user dto.User
id, err := middlewares.GetUserId(c)
log.Println(id)
if err != nil {
return err
}
confmysql.DB.Where("id = ?", id).First(&user)
return c.JSON(user)
}
```
Learning fiber framework and JWT Auth I'm getting in the register Func and Login Func the user Id is saving correctly on DB. The cookie and JWT are getting correctly and disply the cookie and preseve on the frontEnd, but when I want to get the login UserId on Controller I got 0 did some one know's what is happening?
I leave the code hopping have some orientation.
```
// Midleware:
const SecretKey = "secret"
func IsAuthenticated(c *fiber.Ctx) error {
cookie := c.Cookies("jwt")
token, err := jwt.ParseWithClaims(cookie, &jwt.RegisteredClaims{}, func(token *jwt.Token)
(interface{}, error) {
return []byte(SecretKey), nil
})
if err != nil || !token.Valid {
c.Status(fiber.StatusUnauthorized)
return c.JSON(fiber.Map{
"message": "unauthenticated",
})
}
return c.Next()
}
func GetUserId(c *fiber.Ctx) (uint, error) {
cookie := c.Cookies("jwt")
log.Println("Cookie .........: ", cookie)
token, err := jwt.ParseWithClaims(cookie, &jwt.RegisteredClaims{}, func(token *jwt.Token)
(interface{}, error) {
return []byte(SecretKey), nil
})
log.Println("Token .........: ", token)
log.Println("Error .........: ", err)
if err != nil {
return 0, err
}
// var user dto.User
// expireTime := time.Now().Add(24 * time.Hour)
// payloads := jwt.RegisteredClaims{
// Subject: strconv.Itoa(int(user.Id)),
// ExpiresAt: &jwt.NumericDate{Time: expireTime},
// }
payload := token.Claims.(*jwt.RegisteredClaims)
id, _ := strconv.Atoi(payload.Subject)
return uint(id), nil
}
func GenerateJWT(id uint) (string, error) {
expireTime := time.Now().Add(24 * time.Hour)
var user dto.User
token, err := jwt.NewWithClaims(jwt.SigningMethodHS256,
jwt.RegisteredClaims{
Subject: strconv.Itoa(int(user.Id)),
ExpiresAt: &jwt.NumericDate{Time: expireTime},
}).SignedString([]byte(SecretKey))
if err != nil {
log.Println(err)
}
return token, err
}
//Controller:
func User(c *fiber.Ctx) error {
var user dto.User
id, err := middlewares.GetUserId(c)
log.Println(id)
if err != nil {
return err
}
confmysql.DB.Where("id = ?", id).First(&user)
return c.JSON(user)
}
```
Than McIntosh
Oct 11, 2022, 8:36:23 AM10/11/22
to Gerardo Bustani, golang-dev
Hi Gerardo,
This mailing list ("golang-dev") deals with developing and making changes to Go itself, as opposed to using Go to write applications. A better mailing list for your type of question would be "golang-nuts".
Thanks, Than
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/3d3348bb-31d7-42af-82cf-62a0e8cc8294n%40googlegroups.com.
lab...@linux.vnet.ibm.com
Oct 11, 2022, 12:04:57 PM10/11/22
to golang-dev
I'm sorry I was mistaken on the date where it went on the needs-review list so it was more like 10 days.
CL 427634 was the one I was waiting on. It went on the list on Sept. 28 and was given +1 right away but then sat there for some time.
I can't really say for sure on what exactly happened because I didn't go out and watch it closely. At some point it got a merge conflict which maybe took it off the list. When I realized that, I updated the change and then posted a message out here to make sure it got another +1.
Gerardo Bustani
Oct 11, 2022, 1:27:54 PM10/11/22
to Than McIntosh, golang-dev
Thanks for the feedback!
brainman
Feb 7, 2023, 2:46:26 PM2/7/23
to golang-dev
Hello everyone.
I need help with Googler reviews for
The CL was on
list this Sunday, but not anymore.
The CL has +2, and try-bots run, and no unresolved comments. The CL did not change since Sunday.
What did we do wrong?
Thank you.
Alex
Bryan C. Mills
Feb 7, 2023, 3:00:29 PM2/7/23
to brainman, golang-dev
I do see it on the needs-review queue now (2nd from the end).
Not sure why it would have gone missing, but I've seen that with a couple of other CLs over the past month or two too.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/0fdba61f-f855-4e80-8e4a-2c3b1c45ac86n%40googlegroups.com.
Joel Sing
Feb 21, 2023, 11:38:15 PM2/21/23
to golang-dev
On Wednesday, 8 February 2023 at 07:00:29 UTC+11 Bryan C. Mills wrote:
I do see it on the needs-review queue now (2nd from the end).
Not sure why it would have gone missing, but I've seen that with a couple of other CLs over the past month or two too.
As another data point, https://go-review.googlesource.com/c/go/+/415815/8 should be on that list - it was there around 24 hours ago, but has since disappeared.
Michael Pratt
Feb 27, 2023, 12:06:28 PM2/27/23
to Joel Sing, golang-dev
I dug into this a bit and apparently it is due to an issue with Gerrit that sometimes causes stale search results. This is documented on https://gerrit-review.googlesource.com/Documentation/user-search.html: "The change search API is backed by a secondary index and might sometimes return stale results if the re-indexing operation failed for a change update."
There is a plan to fix this in the medium term, but until then I am told that any update on the change (label vote, comment, new patch set, etc) should trigger indexing and fix the staleness, though I haven't had a chance to test that.
--
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/d7e23968-5448-4bde-9375-e1f633d2c541n%40googlegroups.com.
Quim Muntal
Mar 13, 2023, 9:51:26 AM3/13/23
to golang-dev
The change search API sometimes return stale results, and even worse, in some cases, when the CL is updated to trigger indexing, the CL appears in the list for a couple of hours and then disappears again.
Happens in
https://go-review.googlesource.com/c/go/+/471142 and https://go-review.googlesource.com/c/go/+/472195 at least.
By the way, those issues need review :D
El dia dilluns, 27 de febrer de 2023 a les 18:06:28 UTC+1, Michael Pratt va escriure:
brainman
Apr 13, 2023, 5:42:13 AM4/13/23
to golang-dev
Hello,
I don't see the CL on
There are no unresolved comments, and try-bot was run.
Please help.
Thank you.
Alex
Benny Siegert
Apr 13, 2023, 6:02:54 AM4/13/23
to brainman, golang-dev
I added a +1 but there is one more missing. The staleness is an
ongoing issue unfortunately.
--
Benny
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/87b86db2-db10-44b2-ba63-fc3be5768946n%40googlegroups.com.
--
Benny
ongoing issue unfortunately.
--
Benny
--
Benny
Sven Anderson
Apr 27, 2023, 3:27:03 PM4/27/23
to golang-dev
I need a review on
https://go-review.googlesource.com/c/go/+/367296 and it does not show up on needs-review, because the unresolved comments are questions to the _reviewers_.
Sven
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-dev/CAN%2BFjHMd31a_3xbbxHs%2BTXefzP%2Bs8vrWO896ZFfjTxF%2BgYFBcg%40mail.gmail.com.
Ian Lance Taylor
Apr 27, 2023, 3:35:53 PM4/27/23
to Sven Anderson, golang-dev
On Thu, Apr 27, 2023 at 12:27 PM Sven Anderson <sv...@redhat.com> wrote:
>
> I need a review on
> https://go-review.googlesource.com/c/go/+/367296 and it does not show up on needs-review, because the unresolved comments are questions to the _reviewers_.
The needs-review link isn't intended to list every CL that requires
>
> I need a review on
> https://go-review.googlesource.com/c/go/+/367296 and it does not show up on needs-review, because the unresolved comments are questions to the _reviewers_.
review, just the ones that require an additional +1 review from a
Googler.
For your CL, you may have to ping the people to whom you are asking
the question. Sorry for the delay.
Ian
Sven Anderson
Apr 27, 2023, 3:42:14 PM4/27/23
to Austin Clements, Ian Lance Taylor, mkny...@google.com, golang-dev
Ian Lance Taylor <ia...@golang.org> schrieb am Do. 27. Apr. 2023 um 21:35:
On Thu, Apr 27, 2023 at 12:27 PM Sven Anderson <sv...@redhat.com> wrote:
>
> I need a review on
> https://go-review.googlesource.com/c/go/+/367296 and it does not show up on needs-review, because the unresolved comments are questions to the _reviewers_.
The needs-review link isn't intended to list every CL that requires
review, just the ones that require an additional +1 review from a
Googler.
Thanks for the clarification.
For your CL, you may have to ping the people to whom you are asking
the question. Sorry for the delay.
I guess that‘s Austin and Michael K. I added them as explicit addressee now.
Thanks.
Filippo Valsorda
May 24, 2023, 8:30:42 PM5/24/23
to golang-dev
This week I had to kick go.dev/s/needs-review by adding a comment (or privately request a +1) at least four times because the CL was not showing up on the dashboard.
It makes the friction and mental overhead of the Review-Enforcement requirement quite a bit higher, and it's especially bad around a freeze deadline.
Russ Cox
May 24, 2023, 8:49:26 PM5/24/23
to Filippo Valsorda, golang-dev
On Wed, May 24, 2023 at 8:30 PM Filippo Valsorda <fil...@golang.org> wrote:
This week I had to kick go.dev/s/needs-review by adding a comment (or privately request a +1) at least four times because the CL was not showing up on the dashboard.It makes the friction and mental overhead of the Review-Enforcement requirement quite a bit higher, and it's especially bad around a freeze deadline.
It's really bad. My apologies. As you point out, it's an important part of making the Review Enforcement requirement work smoothly.
We have been hoping the Gerrit team will fix it, but we may end up doing our own bot if that doesn't happen.
Best,
Russ
brainman
Dec 31, 2023, 2:06:28 AM12/31/23
to golang-dev
Can a Googler please +1 this CL
so it can be submitted?
The CL is not listed at
for some reason.
Thank you.
Alex
Reply all
Reply to author
Forward
0 new messages