Possible Bug: Label addition / removal affecting unrelated labels to the config

[jessicajaniuk]

We're seeing some inconsistent behavior in which labels that are not in any of the config blocks are also getting removed rather than just the labels we expect. For example there's this PR that for some reason PullApprove removed our cla: yes label. You can find our PullApprove YAML for this specific code path here. It's certainly applying the right label, but it's unclear why it removed other labels. There's one other instance in which the cla label was removed, too.

Here's another PR where we see it removing additional comp:* labels that were manually added. I'm not sure if PullApprove considers other code paths labels when looking at a given PR and removes them if they were added and don't match this code path. That would explain this situation, if that's the case.

This behavior was unexpected for us. Our expectations were that PullApprove would only handle the labels in the config and not affect other labels at all. Is this a correct assumption? Or is this behavior of affecting other labels expected?

[davegaeddert]

Hmm. So it looks like there are two situations here.

1. For the cla: yes situation, I'm guessing this a race condition between the bots. Because we both add and remove labels, we currently use the "set" labels endpoint to do this in one go and I think the CLA bot got in the middle of our operations. I can probably find a fix for that and I'm guessing it's actually unrelated to the other label changes we just did and talked about.
2. For the comp:* situation -- your guess here is correct. PullApprove will remove labels for inactive groups -- so any labels in .pullapprove.yml could potentially be removed if those groups aren't actually active. Again this probably goes back to the original intention of the labels setting... the idea was that you define a label for a given group at a given state, and if the group isn't in that state, it would remove that label (and when the group enters that state, it would add the label).

So it's a combination of both! I can work on the CLA issue you saw (fortunately it looks like your CLA bot resolves that anyway by adding the label back), but I don't have an immediate answer to the other.

[jessicajaniuk]

@davegaeddert Makes sense. I think the latter issue here with the comp: * labels would be resolved with that YAML config suggestion I made to the other issue. If it was just a default label that's added on PR creation and wasn't affected by status changes, there'd be no issue. Either way, these issues are pretty minor.

[davegaeddert]

@jessicajaniuk just to play the "default" idea (#143 (comment)) out a step further... I think there may still be a question about whether pullapprove would be responsible for removing the "default" labels if a group is inactivated.

groups:
  compiler:
    conditions:
      - '"*compiler*" in files'
    labels:
      default:
        - "comp: compiler"

Maybe not the most common scenario, but let's say the PR was opened and included compiler-related changes, but those ended up being irrelevant and pulled back out. If we followed current behavior with labels, the "comp: compiler" label would be removed because that group would no longer be active. Is that would you would expect to happen in that case?

[jessicajaniuk]

We've definitely seen that type of scenario happen here where a subsequent commit on a PR removes a file that was the only file under a label's code path. The behavior you're describing of removing the no longer necessary label would likely be the expected behavior. I know the angular team would be ok with it leaving the label there, too, though, but I'm guessing non-angular teams would prefer the label removal.

[davegaeddert]

Ok, I'll give it some thought as I'm working through other things, but I think that is the main issue with the second situation from above -- the user added labels that PullApprove is trying to manage, so it removed them because those groups aren't actually active.