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

chore(ci): use github app for tokens #6534

Merged
merged 3 commits into from
Nov 28, 2024
Merged

chore(ci): use github app for tokens #6534

merged 3 commits into from
Nov 28, 2024

Conversation

nirinchev
Copy link
Collaborator

@nirinchev nirinchev commented Nov 27, 2024

Description

This is a minor cleanup of gha workflows. It includes the following changes:

  • Bump 3rd party actions to the latest version
  • Use our github app token instead of the service account
  • Cleanup redundant steps or arguments

TODO:

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

id: app-token
with:
app-id: ${{ vars.DEVTOOLS_BOT_APP_ID }}
private-key: ${{ secrets.DEVTOOLS_BOT_PRIVATE_KEY }}

- name: Get GitHub App User ID
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The removed steps in this file have been moved to our setup-bot-token action.

with:
# don't checkout a detatched HEAD
ref: ${{ github.head_ref }}

# this is important so git log can pick up on
# the whole history to generate the list of AUTHORS
fetch-depth: '0'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're not updating AUTHORS in this workflow, so this seems unnecessary


jobs:
merge_bump_packages_pr:
name: Merge bump packages PR
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checkout seems unnecessary here as we don't use anything from the repo

Comment on lines -42 to -43
git add .
git commit --no-allow-empty -m "chore(deps): update electron" || true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The create-pull-request action will automatically add and commit, so no need to do it here

The version of packages is calculated following conventional bumps: See https://github.com/mongodb-js/devtools-shared/tree/main/packages/bump-monorepo-packages for details.
The version of packages is calculated following conventional bumps: See https://github.com/mongodb-js/devtools-shared/tree/main/packages/monorepo-tools for details.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a drive-by - noticed the link is outdated

run: |
npm run bump-packages
git add .
git commit --no-allow-empty -m "$LAST_BUMP_COMMIT_MESSAGE" || true

- name: Create Pull Request
id: cpr
uses: peter-evans/create-pull-request@v6
uses: peter-evans/create-pull-request@5e914681df9dc83aa4e4905692ca88beb2f9e91f # 7.0.5
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For 3rd party packages, we should use git commit shas instead of version tags as those can be force pushed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a documentation for this that we can point to somewhere around this code? It's not obvious and would be very easy to revert back over time without noting why we are doing this

Copy link
Collaborator Author

@nirinchev nirinchev Nov 27, 2024

Choose a reason for hiding this comment

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

It's part of the general security hardening recommendations for actions: https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions

While I don't have reasons to mistrust Peter, a rule of thumb I tend to follow is to use versions for actions by corporations such as microsoft, github, amazon, where there's strong expectation that they have security practices in place that would prevent access to their code. And for everything else, even if it's a popular action, I pin the commit sha.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, so can we add this link somewhere around this line (or any other place you think is appropriate) so that when someone is going over this code in the future and is unsure why exactly this is set to a hash instead of a version the rule is still followed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we should be adding comments as the commit sha usage should be the default for any 3rd party action. I did drop a message on slack though to bring more attention to this. I've also updated CONTRIBUTING.md with a section highlighting a few of the security best practices that also links to the full doc.

@nirinchev nirinchev requested a review from gribnoysup November 27, 2024 01:10
run: |
npm run bump-packages
git add .
git commit --no-allow-empty -m "$LAST_BUMP_COMMIT_MESSAGE" || true

- name: Create Pull Request
id: cpr
uses: peter-evans/create-pull-request@v6
uses: peter-evans/create-pull-request@5e914681df9dc83aa4e4905692ca88beb2f9e91f # 7.0.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a documentation for this that we can point to somewhere around this code? It's not obvious and would be very easy to revert back over time without noting why we are doing this

@nirinchev nirinchev added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Nov 27, 2024
@@ -115,6 +115,14 @@ npm run create-workspace [workspace name]

This will do all the initial workspace bootstrapping for you, ensuring that your package has all the standard configs set up and ready, and all the npm scripts aligned with other packages in the monorepo, which is important to get the most out of all the provided helpers in this repository (like `npm run check-changed` commands or to make sure that your tests will not immediately fail in CI because of the test timeout being too small)

## Using Github Actions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice writeup, thanks

@nirinchev nirinchev merged commit 0fdb9d1 into main Nov 28, 2024
24 of 29 checks passed
@nirinchev nirinchev deleted the ni/gha-tokens branch November 28, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants