search

spring-io / pivotal-cla

Apache License 2.0
10 stars 16 forks source link

Consider Replacing CLA Success with CLA Label #134

Open rwinch opened 8 years ago

rwinch commented 8 years ago

@snicoll Reported:

Would it be possible to tag the issue with a certain label when this comment[1] is added to the issue? And instead of adding yet another comment to confirm it was signed, remove the label and eventually add another one that states the cla is signed.

rwinch commented 8 years ago

My initial impression is that the label doesn't provide a lot of value. You can already filter using status:success to find all Pull Requests that are successful. If you don't like the email that is generated, you can easily filter the emails.

I'm placing this issue for discussion and to see what others impressions are. If it gets a lot of attention, I will consider making the change.

rwinch commented 8 years ago

@wilkinsona - I'm interested in your thoughts since you requested the feature of adding the comment when a CLA needed signing. Obviously this isn't undoing your specific request, but it is fairly related.

wilkinsona commented 8 years ago

Thanks for asking, @rwinch. I may have been a bit too prescriptive when I suggested commenting on the issue when the CLA has been signed.

Stepping back a bit, what I was really looking for was an easy way of knowing that the CLA had been signed. One benefit of a comment over a label is that the comment triggers a notification. That said, I can certainly see the benefit of the label as it makes filtering very easy. I don't think that filtering on status:success is sufficient. The CLA being signed is a blocker for a PR being merged, however other checks may not be (for example, sometimes Travis will fail due to a Checkstyle problem that we'll happily fix while merging the contribution).

Having given this some more thought, I think I now agree with @snicoll and I'd prefer this flow:

  1. A new PR is opened
  2. Comment requesting that the CLA is signed and a requires CLA label is added
  3. User signs the CLA
  4. Requires CLA is removed and CLA signed label is added
snicoll commented 8 years ago

For the record, I am not sure about the "CLA signed label" part. IMO, this will add an unecessary duplication compared to the general status. I'd replace 4 by "Requires CLA label is removed".

Otherwise +1 with the filtering part. That's the main reason why I reported this.

wilkinsona commented 8 years ago

Good point. We could just filter on -label:requires-cla

rwinch commented 8 years ago

@wilkinsona @snicoll Initially my thought was that if the status failed, we aren't interested in the Pull Request anyways. Andy brings up a great point about the fact that we might want to ignore failures like check style. For this reason, I agree about having a label

I sort of like the notification that the CLA was signed. It sounds as though the main goal is for the label to be used. Do either of you have strong opinions as to the comment thanking the user for singing the CLA?

wilkinsona commented 8 years ago

I think it's good to thank the user for signing the CLA, but I also think that we only need to thank them once. I believe they're thanked in the web app when they sign it. If so, I think that's sufficient and also avoids spamming a user with the same comment if they had multiple PRs that were all awaiting the CLA being signed.

rwinch commented 8 years ago

@wilkinsona

I can see the argument that the user might view the email as spam. Personally, I also like the symmetry of thanking them in the comments. This is what I would do if the flow happened manually.

FYI...

When they sign, we don't check for all Pull Requests for that user. This would involve a lot of GitHub API calls. There is no way to find all Pull Requests for a user. Instead we have to retrieve all pull requests for all repositories linked to the CLA tooling. Then iterate over all the pull requests to find any that were performed by this user. See #116 for more details on this.

Instead we only update the pull request that provided the link to the CLA. This means they would only get a single notification. It also makes it apparent which Pull Request was updated.