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

Jump to bottom

x/build/maintner: be confident that returned GerritCL.Status always has one of documented values #24875

Open
dmitshur opened this issue Apr 15, 2018 · 1 comment
Open

x/build/maintner: be confident that returned GerritCL.Status always has one of documented values #24875

dmitshur opened this issue Apr 15, 2018 · 1 comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Apr 15, 2018 •
edited

GerritCL.Status is documented as:

// Status will be "merged", "abandoned", "new", or "draft".
Status string

We want clients of maintner API to be confident in that statement, and not have to doubt it by adding their own "if cl.Status is something else" checks (which is annoying to have to do).

This issue is to verify/confirm that's the case. I believe it is, but @bradfitz asked me to file this in CL 107296:

@bradfitz: Can you file a separate issue about that and I can investigate and ask the Gerrit team if needed?
@dmitshur dmitshur added Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 15, 2018
@gopherbot gopherbot added this to the Unreleased milestone Apr 15, 2018
@dmitshur dmitshur mentioned this issue Apr 15, 2018
Open
@dmitshur
Copy link
Contributor Author

dmitshur commented Nov 1, 2018

@bradfitz, the comment at https://github.com/golang/build/blob/1cecfeb6894c2cbcf619a03313fdf731306caee5/maintner/gerrit.go#L599-L603 says:

// The Go Gerrit site does not really use the "draft" status much, but if
// you need to test it, create a dummy commit and then run
//
//     git push origin HEAD:refs/drafts/master

I tried that just now, and got:

$ git push origin HEAD:refs/drafts/master
Counting objects: 6, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (5/5), done.
Writing objects: 100% (6/6), 835 bytes | 417.00 KiB/s, done.
Total 6 (delta 3), reused 0 (delta 0)
remote: Resolving deltas: 100% (3/3)
remote: Processing changes: refs: 1, done    
To https://go.googlesource.com/build
 ! [remote rejected] HEAD -> refs/drafts/master (private changes are disabled)
error: failed to push some refs to 'https://go.googlesource.com/build'

So, should we still support draft status? In the entire maintner corpus right now, there are only 2 CLs that have "draft" status:

draft: go.googlesource.com/build 38720
draft: code.googlesource.com/gocloud 11714

Both of them show 404 in Gerrit UI for me:

Also, Gerrit API docs at https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-info say that the ChangeInfo.Status field can only have these values:

The status of the change (NEW, MERGED, ABANDONED).

I need to decide how to handle "draft" status here, and given how unhelpful draft CLs are, the best option seems to be to ignore them.

It might be better to do that in maintner so that all of its clients don't have to filter out draft CLs themselves. Unless you think there's some use for them?

@bradfitz bradfitz assigned dmitshur and unassigned bradfitz May 29, 2019
@dmitshur dmitshur removed their assignment Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@bradfitz @dmitshur @gopherbot