-
Notifications
You must be signed in to change notification settings - Fork 16
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
Determine base branch dynamically #1791
Conversation
This reverts commit 571b706.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Re-requesting reviews because I moved the audit and accessibility tests from CircleCI to GitHub Actions. Read the updated PR description for more details |
runs-on: ubuntu-20.04 | ||
services: | ||
postgres: | ||
image: postgres:13.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't provide DockerHub credentials for the postgres and elasticsearch image, but the CircleCI config does:
dockstore-ui2/.circleci/config.yml
Lines 18 to 20 in 5fc1ed6
auth: | |
username: dockstoretestuser | |
password: $DOCKERHUB_PASSWORD |
I assume it was added because of rate-limiting? Do we need it here, and if so, do these credentials already exist in our GitHub secrets (I can't see them)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point there was a concern that DockerHub was going to limit pulls; so we started to add credentials. But then I believe CircleCI made some sort of deal with DockerHub so that pulls from CircleCI wouldn't be rate-limited (or have a higher limit).
My guess is we might be OK; the unauthenticated limit is 100 per 6 hours per IP, and presumably not all our actions run from the same IP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd hope that github has some form of Docker image caching and/or a similar deal.
runs-on: ubuntu-20.04 | ||
services: | ||
postgres: | ||
image: postgres:13.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point there was a concern that DockerHub was going to limit pulls; so we started to add credentials. But then I believe CircleCI made some sort of deal with DockerHub so that pulls from CircleCI wouldn't be rate-limited (or have a higher limit).
My guess is we might be OK; the unauthenticated limit is 100 per 6 hours per IP, and presumably not all our actions run from the same IP.
Let's see over the next couple of months how it performs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this ended up being a lot more work than expected, appreciate powering through it
runs-on: ubuntu-20.04 | ||
services: | ||
postgres: | ||
image: postgres:13.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd hope that github has some form of Docker image caching and/or a similar deal.
Think this is generally ok |
Description
This PR removes
base_branch
from the package.json configand adds a script that can determine the base branch dynamically for PRs. The CircleCI jobs that require the base branch,audit
andaccessibility_test_base
call this script to get the value.Unfortunately there's no built-in way to get the base branch of a PR on CircleCI so the script uses CircleCI'sCIRCLE_PULL_REQUEST
built-in environment variable that contains the PR's URL to extract the PR number then it uses the GitHub API to get the base branch of the PR. Since the script depends on the existence of a PR, the jobs that need a base branch have a step that check if a PR exists before proceeding. If no PR exists, it exits gracefully.While I was in there,
I removed duplicated code for the accessibility test job andI created reusable commands for the repeated commands that I saw.I also created reusable jobs for the integration tests because the only difference between each definition was the integration test group name.
EDIT:
This PR moves the npm audit and accessibility tests from CircleCI to GitHub Actions. GitHub Actions support the
pr_request
event trigger and it provides a built-in environment variable for the base branch of a PR. The workflows will only run when a pull_request event's activity type is opened, synchronize, or reopened. Thus this will not run when the PR is merged to the target branch, which I think is okay because there's not really a "current"/"base" branch when running on the target branch.Noting that in moving the audit test to GitHub Actions, the CircleCI
upload_to_s3
job will happen even if the audit fails. It previously required that the unit tests and audit pass.The CircleCI
audit
job previously was not a required check in GitHub. Should the GitHub Actionaudit
job be a required job? Tagging @denis-yuen because I think it has to be configured via the repo settingsHere are the audit and accessibility GitHub Action runs for this PR:
Review Instructions
Create a PR (or find an existing one) and verify that the audit and accessibility jobs are running via GitHub Actions. Can view the jobs accessibility runs here and the audit runs here.
All CircleCI jobs should also pass.
Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-5531
Security
If there are any concerns that require extra attention from the security team, highlight them here.
Please make sure that you've checked the following before submitting your pull request. Thanks!
npm run build
markdown-wrapper
component, which does extra sanitizationnpm audit
and ensure you are not introducing new vulnerabilities