Fix backport label handler

[apiraino]

Do not allow nominating for backport a PR that is already backport approved.
Revert the -nominated label that was added on GitHub.

r? @Kobzol

thanks

[Kobzol]

I'm not sure if I understand the change, to be honest 😅 Is it about beta nomination not being triggered when the PR is already stable nominated, and vice versa?

I would expect that if we receive IssuesAction::Labeled { label: FOO }, then pr.labels will already contain FOO. Have you checked whether that's the case or not? 🤔

View changes since this review

[apiraino]

I'm not sure if I understand the change, to be honest 😅 Is it about beta nomination not being triggered when the PR is already stable nominated, and vice versa?

I would expect that if we receive IssuesAction::Labeled { label: FOO }, then pr.labels will already contain FOO. Have you checked whether that's the case or not? 🤔

The fix here basically is:

• we have a beta-accepted PR
• we add stable-nominated
• then we add stable-accepted
• bug: the backport.rs handler will return without triggering the Zulip notification handler (notify_zulip.rs) because the label check here does not treat beta and stable backport labels separately.

makes sense?

[Kobzol]

Ah, makes sense, thanks! I think that in the latest commit you just changed the code so that it will simply ignore all labels? pr_labels needs to be checked.

[apiraino]

ah yes you're right, I thought we didn't need it but ofc we do.

Thanks for the review!

[Kobzol]

This will have the same bug as before, right? If the PR is already beta-accepted, it will stop the handler here once stable-accepted is added.

You'll need to separate the labels on the axis of beta/stable, rather than nominated/accepted.

[apiraino]

Hmm ... you're right but the solution is more complex that separating on another axis. I'm afraid I'll need to map all possible cases (4 labels = 16 cases) and then see if I can reduce to a couple of ifs.

In any case, I want to keep it simple: if this does not work, I have another patch to add "negated labels" when sending zulip notification, that would solve the issue from another point of view.

I'll be back on this.

[Kobzol]

Can't just remove this:

if contains_any(&pr_labels, &BACKPORT_LABELS) {
    log::debug!("PR #{} already has a backport label", pr.number);
    return Ok(None);
}

? Seems like the bug is just caused by this.

Btw, I tested that once you receive the Labeled { label: "foo" } event, then pr.labels() already contains foo.

[apiraino]

sorry, I am confused by your comment: isn't that chunk of code removed in this patch? Or did you mean to just remove that code without adding anything else?

In the latter case, I think that not checking labels would open up to confusion when adding regression labels (which was the point of this patch).

[Kobzol]

Yeah, I meant remove without adding anything else. Say that we remove the original check - what will be the problematic situations that could be caused in that case? Adding -nominated once there is already -accepted? Or something else? I don't know what is a "regression label", it wasn't mentioned in this PR so far 😅

[apiraino]

ok @Kobzol I've reworked this patch and tried to simplify as much as possible to keep the scope small and clear (I have edited title and commit desc to reflect this).

Now hopefully the logic should be clearer: with this patch I want to avoid that a backport accepted PR be nominated again. If a -nominated label is added, the handler will revert that. As you suggested I am now checking on the naminated/accepted "axis" instead of the beta/stable.

Note: this won't block creating the Zulip backport poll but it can only be handled in the notify_zulip.rs (another patch will follow).

Let me know if now sounds better. Thanks.