-
Notifications
You must be signed in to change notification settings - Fork 179
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(build): switch to github workflows for robot stack CI #6632
Conversation
Add github workflow definitions for all tasks related to robot stack checks, tests, builds, and deploys that were not previously configured. Specifically, - The api-lint-test and shared-data-lint-test workflows now deploy to pypi when necessary - New workflow js-checks run javascript check actions on any javascript change, since the checkers do not reasonably work on project subsets - New workflow app-test-build runs js tests for the app and its dependencies, cross-platform when necessary; it also builds the app, with full signing support, and deploys it as travis does - New actions for deploying to pypi and setting up environment variables
and appveyor That means removing appveyor entirely :O
Codecov Report
@@ Coverage Diff @@
## edge #6632 +/- ##
=======================================
Coverage ? 73.78%
=======================================
Files ? 10
Lines ? 759
Branches ? 0
=======================================
Hits ? 560
Misses ? 199
Partials ? 0
Continue to review full report at Codecov.
|
This lets us avoid checking in a smooshed-together (Technical term) output and instead take advantage of the github core packages installed by default on the runner.
- name: 'set complex environment variables and get yarn cache' | ||
id: 'set-vars-get-cache' | ||
uses: actions/github-script@v2 | ||
with: | ||
script: | | ||
const { buildComplexEnvVars, findYarnCacheDir } = require(`${process.env.GITHUB_WORKSPACE}/.github/workflows/utils.js`) | ||
buildComplexEnvVars(core, context) | ||
findYarnCacheDir(core, context) | ||
- name: 'cache yarn cache' | ||
uses: actions/cache@v2 | ||
with: | ||
path: ${{ steps.set-vars-get-cache.outputs.yarnCacheDir }} | ||
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }} | ||
restore-keys: | | ||
${{ runner.os }}-yarn- |
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.
Out of curiosity, instead of grabbing the global yarn cache from your util, why not just have a step that sets the cache directory like here?
Also, I don't see where behavior changes when there's a cache hit, you're still going to do a yarn
from inside of setup-js
even if there's a cache hit right?
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.
Yeah but that should prevent yarn from redownloading everything shouldn't it?
In terms of why put it in a file, it accomplishes essentially the same thing and the file really had to be there for the custom env vars so I built it in anyway
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.
Yeah but that should prevent yarn from redownloading everything shouldn't it?
Ah yeah that makes sense. Is it working as expected now? Speaking of which, how are you validating that this works? I assume just looking through the action logs and making sure yarn doesn't redownload everything?
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.
Nvm, can see it's working now. Nice! https://github.com/Opentrons/opentrons/runs/1193602613?check_suite_focus=true
It looks like these lines are being copied and pasted a few times (I'm going to be using them too in my e2e PR). Maybe it's worth having this be its own task or something?
|
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.
This is looking really cool, and thanks for waiting for me to get this review in. I've got some questions in comments, as well as one extra one:
- Should
.github/workflows/api-lint-test.yaml
be renamed to reflect that it is also deploying?
SHORTSLUG=`echo ${{ github.ref }} | sed -e "s/^\/\?refs\/\(?:tags\|heads\)\///"` | ||
echo "Found ref slug ${SHORTSLUG} from ref ${{ github.ref }}" | ||
aws s3 sync ${{ inputs.distPath }} s3://sandbox.${{ inputs.domain }}/${SHORTSLUG} --acl=public-read | ||
aws s3 sync ${{ inputs.distPath }} s3://sandbox.${{ inputs.domain }}/${{ inputs.destPrefix }} --acl=public-read |
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.
Prior to switching PD, we will need to confirm that this is successfully uploading dotfiles in distPath
to S3, which is behavior PD relies on
Also cache npm prebuilds
They should now require tests to pass. The app ones also don't rebuild but instead pull the prebuilt apps.
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.
This PR switches robot stack check, test, build, and deploy workflows to Github Actions, implementing the CI tasks that were not previously covered in other PRs. This includes
Please look it over and poke holes in it.
This DO NOT MERGE tag is here to remind us that we SHOULD NOT MERGE THIS until we switch the app deploy S3 prefix back to the standard one.