-
Notifications
You must be signed in to change notification settings - Fork 39
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
ci: speed up workflows and reduce costs #1545
Conversation
8124b92
to
d279ced
Compare
d279ced
to
01260d0
Compare
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
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.
Didn't review changes under packages/
much but the CI stuff looks good to me! Couple of small changes to make
all = true | ||
keepBytes = 30000000000 # 30 GB | ||
keepDuration = 864000 # 10 days | ||
gc = false |
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.
it means the cache size will grow indefinitely
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.
We drop it every time. Only cargo registry mount is stored on S3 where we have own retention policy for files (3 days)
shell: bash | ||
run: echo "sha=$(git log -1 --format="%h" -- packages/dashmate)" >> $GITHUB_OUTPUT | ||
|
||
# TODO: Use upload artifacts action instead |
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.
check this todo
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 wasn't going to do it this iteration
key: local-network-volumes/${{ steps.dashmate-fingerprint.outputs.sha }} | ||
if: steps.local-network-data.outputs.cache-hit != 'true' | ||
|
||
- name: Configure pre-built docker images |
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.
Where do we get these images from? Looks like we assume they are already pushed (= we should have some dependency on another job declared above). Please add dependency/comment explaining that.
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.
It's not a job, it's an action that pulls images to do the work. Workflows what use this action defines dependencies to build-images job.
- name: Cache NPM build artifacts (S3 bucket cache) | ||
uses: everpcpc/actions-cache@v1 | ||
if: contains(runner.name, 'ubuntu-platform') | ||
uses: strophy/actions-cache@opendal-update |
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.
We should prefer @master
, + we should migrate that repo to dashpay org
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.
We expect some updates on upstream soon. When they don't we will change it.
- name: Install protoc | ||
id: deps-protoc | ||
shell: bash | ||
run: | | ||
curl -Lo /tmp/protoc.zip \ | ||
https://github.com/protocolbuffers/protobuf/releases/download/v22.0/protoc-22.0-linux-x86_64.zip | ||
https://github.com/protocolbuffers/protobuf/releases/download/v22.0/protoc-22.0-linux-aarch_64.zip |
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.
Will it work on different architectures? maybe we should add some if depending on runner arch?
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.
It works this way according to the tests. We want to move this step to AMI. When infra guys have time they will do it.
target: ${{ inputs.target }} | ||
platform: linux/arm64 | ||
push_tags: true | ||
dockerhub_username: ${{ secrets.DOCKERHUB_USERNAME }} |
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.
why we use dockerhub_username if we use ECR? If this is correct (eg. just var name), please add comment
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.
We login to our pairs docker hub user to extend pull limits
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.
Great job guys!
Issue being fixed or feature implemented
What was done?
DASHMATE_E2E_TESTS_SKIP_IMAGE_BUILD
in to disable image builds in dashmate testsDASHMATE_E2E_TESTS_LOCAL_HOMEDIR
to set specific home dir in dashmate testsBROWSER_TEST_BATCH_INDEX
andBROWSER_TEST_BATCH_TOTAL
to run only batch of test suite tests in browsersHow Has This Been Tested?
Running workflows
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only