search

spring-io / pivotal-cla

Apache License 2.0
10 stars 16 forks source link

Duplicate requests to sign the CLA #136

Open wilkinsona opened 8 years ago

wilkinsona commented 8 years ago

I've only seen it once. See https://github.com/spring-projects/spring-boot/pull/3963

Shredder121 commented 8 years ago

Coule you look up the corresponding payloads? Maybe the label action of spring-issuemaster triggered also an update? Although I must admit, they are a lot of time apart..

Maybe making the handling occur on one and only one thread will solve possibilities like these?

wilkinsona commented 8 years ago

Maybe the label action of spring-issuemaster triggered also an update?

That seems unlikely. The label was applied by Spring Issuemaster before the first comment from Pivotal Issuemaster.

Shredder121 commented 8 years ago

At least, you would think that something must have triggered it? Maybe the corresponding payload could be looked up (really wish that page had a filter function, or search).

But yeah, very unlikely that it was that specifically.

rwinch commented 8 years ago

@wilkinsona Thanks for the report!

I'm not sure what would have caused this at the moment. The comments are at 2:49 am CDT & 3:49 am CDT which alludes to the fact that it is not a race condition.

Looking at https://status.github.com it appears everything has been operational.

In short, I'm not sure what caused this but we will keep it open until we get it sorted out or haven't seen it in a long time.

wilkinsona commented 8 years ago

Here's another example of a duplicate request: https://github.com/spring-projects/spring-framework/pull/1120

snicoll commented 8 years ago

And the second message was posted a while after the PR was closed...

mp911de commented 8 years ago

Do you have access to the event notification payload? The PR was updated at 2016-07-26T12:09:00 MESZ

Shredder121 commented 8 years ago

@mp911de Also note, that we don't find the existing comment. The comment could have been updated.

mp911de commented 8 years ago

What currently bugs me is that we have two guards in the code and none of them seems to work. We usually don't update closed PR's

wilkinsona commented 8 years ago

Assuming I'm looking at the right place in the code, the logic is an OR so only one of the guards needs to be wrong for the comment to be made.

mp911de commented 8 years ago

The reason is that the body has in each case the body is slightly different. The first PR has a difference in the links (https://cla.pivotal.io/sign/spring vs. https://cla.pivotal.io/sign/spring?repositoryId=spring-projects/spring-boot&pullRequestId=3963). The second PR uses different user names. That's one riddle solved. Now I just need to find out why we still put comments on closed pull requests.

Shredder121 commented 8 years ago

@mp911de yeah I noticed that, thanks for looking into it.

@wilkinsona the pull request state is probably either open or closed, Could you find the corresponding webhook payload? The pull_request property should reflect what state the pull request was in.

mp911de commented 8 years ago

I'm fixing it right now and constraining comment interaction to open pull requests only.

wilkinsona commented 8 years ago

Could you find the corresponding webhook payload?

How do I do that?

mp911de commented 8 years ago

Go to the Repository Settings -> Webhooks & services, select https://cla.pivotal.io/github/hooks/pull_request/pivotal. You'll see in the lower section of the page Recent Deliveries

wilkinsona commented 8 years ago

I guess that's only useful for spring-projects/spring-framework#1120? I don't have access to the repository settings for Spring Framework. @snicoll might be able to help.

mp911de commented 8 years ago

I think I found also the other reason why the second comment was added. It's a combination of unknown and the manual sync. The manual sync uses the username of the user logged into Pivotal CLA. It also uses the unknown status to propagate a sync. That are the reasons why spring-projects/spring-framework#1120 got commented twice.

Shredder121 commented 8 years ago

It's a combination of unknown and the manual sync.

Can it be that the repository was deleted?

mp911de commented 8 years ago

ehlxr wants to merge 1 commit into spring-projects:master from unknown repository

That's another issue here. So much coincidence in one PR.

Shredder121 commented 8 years ago

Haha, I believe that's called "field testing". But yeah, it's unfortunate, so many things to go wrong.

I tried to find the corresponding event in the events API, but it's not published there unfortunately. So the information has to come from the webhook payload I suppose.

snicoll commented 8 years ago

I have access to recent deliveries if someone needs to look at that.

mp911de commented 8 years ago

I fixed the discovered issues with duplicate comments. Let's keep the ticket open for a few more days. We can close it at Spring One if duplicates don't pop up anymore.

Shredder121 commented 8 years ago

Just wondering/verifying, S1 means SpringOne right?