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

Adding CLA Assistant action workflow #48249

Merged
merged 11 commits into from
Nov 19, 2024
Merged

Conversation

doggydogworld
Copy link
Contributor

@doggydogworld doggydogworld commented Oct 31, 2024

Overview

We currently use the 3rd party service CLA Assistant to automate the handling of CLA signatures. Using a 3rd party service comes with downsides. In the case of a service interruption our PRs are blocked until the stability of the service is reestablished.

There is a Contributor Assistant Action that has similar functionality and will run as an action. This should alleviate any concerns of service interruption (unless GitHub goes down) and also allows us to add features as needed.

Implementation

Added a new workflow that kicks off for PR events. This will detect if any changes were introduced by someone who hasn't signed the CLA and will post a comment with instructions.

Signatures are stored in the cla-signatures repository and are updated by the workflow.

Security Considerations

This introduces two new secrets into the OSS repo:

  • CLA Assistant App ID
  • CLA Assistant App Private Key

These combined can be used to perform authenticated REST API calls.

The GitHub App associated with the above is only installed on the cla-signatures repo and has the following access:

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48249.d3pp5qlev8mo18.amplifyapp.com

@doggydogworld doggydogworld added the no-changelog Indicates that a PR does not require a changelog entry label Nov 1, 2024
@doggydogworld doggydogworld marked this pull request as ready for review November 1, 2024 20:17
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
# If the user is a member of the org expect a 204 HTTP Status Code
# If the above is success the process will exit 0
run: |
gh api "orgs/gravitational/members/${USER}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
gh api "orgs/gravitational/members/${USER}"
gh api "orgs/gravitational/members/${USER}" > /dev/null

This doesn't contain much sensitive information currently, but would probably be a good idea to either capture the output or trash it in case something changes in the future

.github/workflows/cla-assistant.yaml Outdated Show resolved Hide resolved
Comment on lines 3 to 4
issue_comment:
types: [created]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this trigger on issues too? We'd probably want to avoid that and only trigger on PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately from GitHub's perspective PR comments ARE issue comments. Hmm I could make it do a noop for "true" issue comments so it exits fast. Since this is a public repo I believe this should be no cost.

issue_comment:
types: [created]
pull_request_target:
types: [opened, closed, synchronize]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need closed event? Also, when does synchronize trigger?

run: |
gh api "orgs/gravitational/members/${USER}"
- name: "CLA Assistant"
if: steps.get-membership.outcome != 'success' && ((github.event.comment.body == 'recheck' || github.event.comment.body == 'I have read the CLA Document and I hereby sign the CLA') || github.event_name == 'pull_request_target')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I fully understand this condition. Looks like it checks if the comment says either "recheck" or "I have read ..." and adds them to the signers repo? Does it check that the comment was left by the PR author?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it is a bit vague. I'll add some comments to describe the expected behavior.

Essentially though the behavior for anyone outside gravitational org is:

  • CLA Assistant gathers authors from commits made to the PR
  • If any author has not signed CLA it fails.

In the case of a failure the DX is:

  • Reply with "I have read..."
    • The action will add the username to the cla-signatures list and "recheck" automatically
  • "recheck" (can be commented to manually rerun in the case of a transient error)
    • Gathers authors from all commits in PR and checks cla-signatures

Copy link
Contributor

@fheinecke fheinecke left a comment

Choose a reason for hiding this comment

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

Approved pending Roman's comments

@r0mant
Copy link
Collaborator

r0mant commented Nov 6, 2024

@doggydogworld Let's run both this one and old cla-assistant check in parallel for a few days and test external contributions before making this one required and removing the old one.

@doggydogworld doggydogworld added this pull request to the merge queue Nov 19, 2024
Merged via the queue into master with commit aaf8cd9 Nov 19, 2024
40 checks passed
@doggydogworld doggydogworld deleted the gus/cla-assistant-gha branch November 19, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants