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

x/build/cmd/gopherbot: don’t comment on issues with WIP CLs #30113

Closed
andybons opened this Issue Feb 6, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@andybons
Copy link
Member

andybons commented Feb 6, 2019

A Work-in-Progress CL was created for #29984, and it was attached to the issue. If a change is WIP, it shouldn’t add the CL comment until it’s ready for review.

@rsc @golang/osp-team

@gopherbot gopherbot added this to the Unreleased milestone Feb 6, 2019

@gopherbot gopherbot added the Builders label Feb 6, 2019

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Feb 6, 2019

There's not enough information here for me to understand this issue. Could you please clarify.

Per https://golang.org/wiki/CodeReview (and Interim Code Review and Issue Tracker Conventions), we support marking a CL as a work-in-progress by including the text "DO NOT REVIEW" in the commit message.

CL 160458, which mentions issue #29984 (I'm not sure if it's the same CL you were referring to) did not include such text, but it used the Gerrit "WIP" bit.

Is this issue about adding official support for that Gerrit feature as a way of marking a CL as WIP?

Or is it about gopherbot treating CLs with "DO NOT REVIEW" text in them differently?

@dmitshur dmitshur added WaitingForInfo and removed NeedsFix labels Feb 6, 2019

@andybons

This comment has been minimized.

Copy link
Member Author

andybons commented Feb 6, 2019

If a CL is uploaded to Gerrit that is marked at Work in Progress (WIP), then gopherbot shouldn't post a message on the issue saying "golang.org/cl/NNN mentions this issue" until it moves out of WIP mode.

This is not about DO NOT REVIEW text. We can check the bit using https://godoc.org/golang.org/x/build/maintner#GerritCL.WorkInProgress

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Feb 6, 2019

Thanks for clarifying.

@dmitshur dmitshur added NeedsFix and removed WaitingForInfo labels Feb 6, 2019

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Feb 7, 2019

Change https://golang.org/cl/161597 mentions this issue: cmd/gopherbot: Don’t comment on issues with WIP CLs

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Feb 7, 2019

If a CL is uploaded to Gerrit that is marked at Work in Progress (WIP), then gopherbot shouldn't post a message on the issue saying "golang.org/cl/NNN mentions this issue" until it moves out of WIP mode.

Why not?

What does the WIP bit really mean? It obviously doesn't mean "don't advertise this" because otherwise you wouldn't have uploaded it. So why not advertise it more at that point and potentially avoid people wasting redundant effort on the same CL?

@andybons

This comment has been minimized.

Copy link
Member Author

andybons commented Feb 7, 2019

It is meant to not advertise it (emails are not sent for WIP changes). It was created so that people could upload changes that were not ready for broad review or even just so they could upload it and download it on another computer to continue their work.

Check out https://gerrit-review.googlesource.com/Documentation/intro-user.html#wip for more information

@andybons

This comment has been minimized.

Copy link
Member Author

andybons commented Feb 8, 2019

Closing in favor of not having duplicate effort in CLs. I didn’t think about that point.

@andybons andybons closed this Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment