Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CLA Signature bot #9310

Merged
merged 5 commits into from
Aug 26, 2020
Merged

Add CLA Signature bot #9310

merged 5 commits into from
Aug 26, 2020

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Aug 25, 2020

The CLA signature bot will check the authors of each PR to ensure they have all signed the CLA. If any authors still need to sign the CLA, it will leave a comment explaining how it can be signed, and will check back upon each comment to see if it has been signed.

The bot used is Metamask/cla-signature-bot, which is a fork of Roblox/cla-signature-bot. The fork has a couple of improvements, and it updated the PR comment text to be more appropriate for our usage.

Currently the only whitelisted user is dependabot, meaning that even ConsenSys employees will need to sign the CLA. We could add all employees to the whitelist, but I figured it'd be easier to get everyone to sign individually rather than maintaining this list here.

The signatures are stored in cla.json on the cla-signatures branch, which is in this repository as a distinct root. We can consider moving this to a separate repository in the future - this was just easier to setup.

The CLA signature bot will check the authors of each PR to ensure they
have all signed the CLA. If any authors still need to sign the CLA, it
will leave a comment explaining how it can be signed, and will check
back upon each comment to see if it has been signed.

The bot used is `MetaMask/cla-signature-bot`, which is a fork of
`Roblox/cla-signature-bot`. The fork has a couple of improvements, and
it updated the PR comment text to be more appropriate for our usage.

Currently the only whitelisted user is `dependabot`, meaning that even
ConsenSys employees will need to sign the CLA. We could add all
employees to the whitelist, but I figured it'd be easier to get
everyone to sign individually rather than maintaining this list here.

The signatures are stored in `cla.json` on the `cla-signatures` branch,
which is in this repository as a distinct root. We can consider moving
this to a separate repository in the future - this was just easier to
setup.
@Gudahtt Gudahtt marked this pull request as ready for review August 25, 2020 20:44
@Gudahtt Gudahtt requested a review from a team as a code owner August 25, 2020 20:44
@Gudahtt Gudahtt requested a review from rekmarks August 25, 2020 20:44
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 25, 2020

Note that actions haven't yet been enabled on this repo, and even if they had, it wouldn't work properly yet because of my use of the pull_request_target event trigger (which only uses actions that are in the base branch).

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 25, 2020

Currently the workflow for first-time contributors/signers requires the action to be manually re-run unfortunately. It's supposed to update automatically after the author signs, but it doesn't seem to be working correctly. Instead of re-running, the job gets queued up again but never executes (probably a GitHub bug? 🤔). Manually re-running once per contributor isn't terrible though.

@metamaskbot
Copy link
Collaborator

Builds ready [6015a3f]
Page Load Metrics (486 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30103442110
domContentLoaded31468648413665
load31568748613665
domInteractive31468548413666

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 25, 2020

I have just added a branch protection rule for the cla-signatures branch. I couldn't enable any of the options like requiring status checks (as the action must be allowed to push directly to that branch), but at least it prevents deletion and force pushes and non-linear history.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 25, 2020

Note that actions triggered by the pull_request_target event are automatically added as status checks. We can make this a required status check after this is merged.

Screenshot:

status_check

.github/workflows/cla.yml Outdated Show resolved Hide resolved
rekmarks
rekmarks previously approved these changes Aug 25, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one language-related nit.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 25, 2020

I just noticed that I'm seeing this problem on my fork when testing this: Roblox/cla-signature-bot#10

I'm going to try to fix this as well. This would really clutter up the actions tab.

@metamaskbot
Copy link
Collaborator

Builds ready [5c16eda]
Page Load Metrics (485 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298440157
domContentLoaded26469448314369
load26669648514369
domInteractive26469448314369

The version of `MetaMask/cla-signature-bot` has been updated from
`v3.0.0` to `v3.0.1`. This version is more tolerant of being run
against issue comments, and it has replaced the `whitelist` input
parameter with `allowlist`.
@Gudahtt Gudahtt marked this pull request as ready for review August 26, 2020 01:56
@Gudahtt Gudahtt marked this pull request as draft August 26, 2020 01:59
@Gudahtt Gudahtt marked this pull request as ready for review August 26, 2020 02:04
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 26, 2020

This should be ready to review again now. I've addressed the issue comment problem in 5c16eda by skipping the job if it was triggered by an issue comment rather than a PR comment. Unfortunately this still ends up showing up in the 'Actions' tab, but at least it's greyed out. This was the best solution I could find. It doesn't seem possible yet to trigger actions just on pull request comments but not on issue comments.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 26, 2020

Also I'd appreciate a review of the commits I've made on cla-signature-bot: https://github.com/MetaMask/cla-signature-bot/commits/master

Most of them have accompanying PRs here: https://github.com/Roblox/cla-signature-bot/pulls
The one exception is MetaMask/cla-signature-bot@83d45d2 - which isn't yet sufficiently generalized to submit back upstream.

@metamaskbot
Copy link
Collaborator

Builds ready [485431c]
Page Load Metrics (510 ± 83 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint309942168
domContentLoaded29283350817383
load29383551017383
domInteractive29283350817383

rekmarks
rekmarks previously approved these changes Aug 26, 2020
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

A new input parameter, `allowOrganizationMembers`, has been added in
`v3.0.2` of `MetaMask/cla-signature-bot`. This parameter automatically
includes all organization members in the allowlist.
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 26, 2020

I have added one more feature to cla-signature-bot: the allowOrganizationMembers flag. This ensures that none of us need to sign the CLA.

@metamaskbot
Copy link
Collaborator

Builds ready [0dc2f2e]
Page Load Metrics (503 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3197442010
domContentLoaded26175550115273
load26375650315273
domInteractive26175550115273

rekmarks
rekmarks previously approved these changes Aug 26, 2020
@whymarrh
Copy link
Contributor

whymarrh commented Aug 26, 2020

I've enabled local actions for this repo—I had disabled actions across all the repos a while back before we'd decided to start using them so as we add workflows we'll need to enable it.

Where we're using our fork I've left it at just local actions but it might even be good to try and stick to local actions so we don't have random third-party code running on CI.

The `allow-organization-members` input parameter was incorrectly named.
It has been fixed. The version of the `cla-signature-bot` has been
updated as well, because I have reset the tags used on that repo.
`v3.0.0` now refers to the latest version.
@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 26, 2020

I'm going to disable actions again for now until we get final signoff on this CLA.

I'm not sure if we need to enable third-party actions for issue_comment to work correctly 🤔 I suppose even PRs from forks are considered a part of this repository? I guess we'll find out.

@whymarrh
Copy link
Contributor

I think that simply restricts where we can uses code from. That is, our uses: MetaMask/[email protected] versus uses: roblox/[email protected].

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 26, 2020

Note that I've just pushed one more commit to this branch (hopefully the final one 🤞 ). I had discovered a couple of mistakes when testing, and they are now fixed.

Also I've discovered that the allow-organization-members flag only works for anyone who is a public member of the organization. This means that any private members will need to either become public or sign the CLA. This seems fine though - if anything it's a good thing, as now we aren't leaking private information via this bot's behavior.

@metamaskbot
Copy link
Collaborator

Builds ready [85b03a6]
Page Load Metrics (481 ± 65 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298039115
domContentLoaded26071347913565
load26171548113565
domInteractive26071347913565

@Gudahtt Gudahtt merged commit 885bd13 into develop Aug 26, 2020
@Gudahtt Gudahtt deleted the add-cla-signature-bot branch August 26, 2020 16:48
@Gudahtt
Copy link
Member Author

Gudahtt commented Sep 2, 2020

I have just enabled local Actions for this repository (as described in this comment), so the bot should start posting now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants