Confusion converting labels to submit-requirements

[Clark Boylan]

I've begun looking at preparation tasks for an eventual Gerrit 3.6 to
3.7 upgrade. One thing we'll need to change is our use of blocking
label functions converting them to submit requirements. I've found the
label function documentation [0] to be confusing making it difficult
to know if I've properly updated our labels.

In particular, the docs say that label.name.function is deprecated,
but we must still set the function to NoBlock/NoOp. This has me
wondering why set any function value at all if this configuration item
is deprecated and going away eventually? This means that we'll have to
update this configuration twice rather than once. Then I noticed the
documented default is MaxWithBlock which is supposed to no longer be
allowed. However, it does appear that I am able to push a custom label
definition with no function set. According to the docs I should be
getting the old MaxWithBlock behavior? Why is the default value a
disallowed value? And why does Gerrit allow me to push a config that
will use the default value?

Put another way is setting the function actually required because
otherwise MaxWithBlock is chosen? If so, is this a bug? Should the
default be changed to a valid option instead? Should Gerrit reject
pushes of custom labels that don't set a function? And is
label.name.function actually going away in the future? Maybe we can
make operator lives a little easier allowing them to remove it
entirely when they convert to submit requirements?

I'm happy to help update documentation (and possibly Gerrit itself
depending on how involved this is) to make this more clear if given
some clarification.

[0] https://gerrit-review.googlesource.com/Documentation/config-labels.html#label_custom

Clark

[Ian Wienand]

On Wed, Jan 25, 2023 at 9:09 AM Clark Boylan <clark....@gmail.com> wrote:
> Put another way is setting the function actually required because
> otherwise MaxWithBlock is chosen?

It seems to me that MaxWithBlock is the default when creating a new label [1]

The checking for pushing new changes is done at [2], it seems like if
you're not changing the
label function, this will be skipped.

I can not see that at [3] this wouldn't be converted into a separate label, e.g.

submitableIf = label:<name>=MAX

From my reading of [4], if you have *also* defined label:<name> that's
OK, but it will take
the one that is blocking in preference. So it seems if your label
were *less* that MAX, it would
be essentially ignored for the MaxWithBlock behaviour.

> If so, is this a bug? Should the default be changed to a valid option instead?

I'm not sure about a bug, but certainly something hard to get right ...

It does seem that switching the default label function to NoOp might
be a regression,
if you were assuming that your new label should block submission. You
can probably construct
a scenario where some sort of automation rolls out gerrit and sets up
labels, and suddenly
what should be a blocking label does nothing (because the default
changed to NoOp)?

Instead of switching it, you could just error out if the label doesn't
explicitly have a function, which
should at least break the above scenario loudly and force the user to
think about it. But that seems a
little crazy to *require* a deprecated argument...

Maybe something like a sentinel function value needs to be made for
the default case? A value that
the user can't specify, but indicates that no function was set on the
label creation. Then the adapter
looks for submit requirements rules for that label first, and if they
don't exist, then falls back to
MaxWithBlock behaviour? That seems like it lets you both not set a
function in the label and set your
own submit rules, but retains the old behaviour of MaxWithBlock for
existing label creations?

-i

[1] https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.7.0/java/com/google/gerrit/entities/LabelType.java#123
[2] https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.7.0/java/com/google/gerrit/server/project/LabelConfigValidator.java#182
[3] https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.7.0/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
[4] https://gerrit.googlesource.com/gerrit/+/refs/tags/v3.7.0/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java#82