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

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

Comments

@dmitshur
Copy link
Member

commented Apr 15, 2018

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

This comment has been minimized.

Copy link
Member Author

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
Projects
None yet
3 participants
You can’t perform that action at this time.