spring-io / pivotal-cla

Apache License 2.0
10 stars 16 forks source link

Provide a mechanism to indicate that a change is considered obvious #135

Closed mp911de closed 8 years ago

mp911de commented 8 years ago

Users can now add a comment (issue comment/review comment within a commit) that the change contains an "obvious fix". We no longer require the CLA to be signed. In case the CLA is signed we keep the regular flow and update the status up CLA signed. If the user is a first-time user and the PR has a comment saying "Please sign..." we add a disarming comment stating "CLA signing not necessary" and update the commit status to "CLA not required".

Fixes gh-132

Shredder121 commented 8 years ago

I think we're still missing the "obvious fix" text that could be present in the PR body (PullRequestEvent opened/edited). Or am I not looking carefully enough?

mp911de commented 8 years ago

Good catch. I added another commit to catch obvious fix in the PR body.

Shredder121 commented 8 years ago

And what about the PR title?

I'm sure there are people who would name their PRs something like [Obvious fix] flip null check.

Shredder121 commented 8 years ago

As a side note, are lombok's field defaults not working for you? Just wondering.

mp911de commented 8 years ago

I checked some PR's inside various repositories. There are no PR's named with that pattern. I think we should not allow too much possibilities up-front.

Not sure I follow regarding

are lombok's field defaults not working for you?

Shredder121 commented 8 years ago

I think we should not allow too much possibilities up-front.

Fine by me, you're right.

Not sure I follow regarding

are lombok's field defaults not working for you?

As of #104 lombok helps making fields private by default, making private declarations not necessary anymore. I saw you added the access modifiers in for example PullRequestId, which is okay, a matter of taste I suppose, although @Value already makes every field private and final. But, I also noticed that you put back a few private keywords which lead me to ask that question.

mp911de commented 8 years ago

Got it. That's just a matter of not paying enough attention to the recent changes. Thanks for the hint.

mp911de commented 8 years ago

The IntelliJ plugin has an issue with the lombok.config. Errors get displayed only after compiling. D'oh!

Shredder121 commented 8 years ago

The IntelliJ plugin has an issue with the lombok.config.

Yeahh, that's possible.

Sorry for the nagging though.

mp911de commented 8 years ago

I found and fixed a bug that was located in filtering "own" events. The check now considers the sender field to check the user whether to skip the event.

Shredder121 commented 8 years ago

The check now considers the sender field

Yes sounds good. Although, what do you do if the pull_request event is sent by a bot, instead of the user? Then would the bot need to submit a CLA? Or would you also take the git author in account?

Shredder121 commented 8 years ago

Hmm, nevermind, didn't look good enough. It's a little late, my bad.

mp911de commented 8 years ago

The current thinking is: The creator of the PR (= the user who opens the PR) should have signed the PR. There are however other aspects to that: What about the committing user? What if there are multiple commits from different people?

As long as we don't consider the commits we just care whether the user, who opened the PR, has signed the CLA - no matter whether it's a bot or not.

rwinch commented 8 years ago

Thanks for the PR! This is merged via 1ff2eff7f695c0cb9044aff4fd194d6718a4021a