search

flokli / gerrit-queue

A merge bot for Gerrit
Apache License 2.0
17 stars 2 forks source link

gerrit: Use a Gerrit label instead of hashtag for autosubmit #11

Closed flokli closed 2 years ago

flokli commented 2 years ago

This moves to using a Gerrit label ('Autosubmit') with boolean values for determining whether a developer wants to have a change automatically submitted.

See also https://cl.tvl.fyi/c/depot/+/4172

tazjin commented 2 years ago

LGTM (well, I wrote it :wink:)

tazjin commented 2 years ago

Based on the specific configuration of an instance you may also not see this label on instances where it is present in the config (e.g. if it's not indicated as may(_) in prolog rules). In TVL the behaviour of gerrit-queue in that case is that its submittability criteria don't trigger and nothing happens.

flokli commented 2 years ago

Yeah, I was referring to the logic not being very approachable. I added a unit test which exercises the IsAutosubmit function with various goGerrit.ChangeInfo (or more correct, the resulting Changeset) structs.

flokli commented 2 years ago

@tazjin how do you configure gerrit to only allow owners to set the label? is that rules.pl as well?

I added something to the docs, but it's a bit squishy - would love to have a more correct explanation there.

tazjin commented 2 years ago

No, it's not in rules.pl - it's just standard Gerrit access configuration. See cl/4202 for access setup.

Note that if a setup uses rules.pl, the label will not be rendered unless it is configured as may(_) in the rules. See cl/4241.

flokli commented 2 years ago

Thanks for linking to these CLs! I added some docs on how to configure it in the README.md - and by this, realized the label normally is configured to only be 0 or -1.

How does a goGerrit.ChangeInfo look like if the value is set to 0 (either keeping the default, or explicitly toggled to 1 and then 0 again)? Is the label simply not present?

https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#_fields_set_by_code_labels_code doesn't seem to be too verbose in how this is expressed…

It suggests we should look in the value if it's not +1 or -1, and that doesn't seem to match what labelInfoToInt is doing…

tazjin commented 2 years ago

My take is that the only assumption you can make is that Autosubmit = 1 means "please autosubmit". Your labelInfoToInt function returns that value correctly, everything else it may or may not return doesn't matter.

Any other case (e.g. label not being present, label being set to 42 or -17, label being present at 0 but with may(_) or need(_), ...) is custom logic of the Gerrit instance and not something you can rely on for any logic.

flokli commented 2 years ago

Yes, for the Autosubmit functionality, only a ChangeInfo with the "Autosubmit" label info and an "Approved" will have labelInfoToInt return 1, which will cause IsAutosubmit() to return true.

However, all labels with values != [-2, -1, 0,1, 2] will apparently cause labelInfoToInt to return 0, which is wrong. I added a comment to the labelInfoToInt function to document this.