-
Notifications
You must be signed in to change notification settings - Fork 1k
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: Use buildkit caching when building Dockerfile.ci #3428
Conversation
I think this needs to be rebased on main before it'll work? |
496b814
to
a35683c
Compare
This can be simplified once #3430 lands |
.github/workflows/ci.yml
Outdated
if [ -n "${{ secrets.DOCKER_USERNAME }}" ] && [ -n "${{ secrets.DOCKER_PASSWORD }}" ]; then | ||
echo ${{ secrets.DOCKER_PASSWORD }} | docker login -u ${{ secrets.DOCKER_USERNAME }} --password-stdin | ||
if [[ -n "${{ secrets.GHCR_PAT }}" ]]; then | ||
echo "${{ secrets.GHCR_PAT }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin |
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.
Tried to use the GITHUB_TOKEN
here but doesn't seem to work even after giving the repo write access to the org package
"cd /home/dependabot/dependabot-core/bundler/helpers/v2 && BUNDLER_VERSION=2 bundle install && BUNDLER_VERSION=2 bundle exec rspec spec" | ||
- name: Run ${{ matrix.suite.name }} tests | ||
run: | | ||
docker run --env "CI=true" --env "DEPENDABOT_TEST_ACCESS_TOKEN=$DEPENDABOT_TEST_ACCESS_TOKEN" --env "SUITE_NAME=${{ matrix.suite.name }}" --rm "$CORE_CI_IMAGE" bash -c \ | ||
docker run \ | ||
-v "$(pwd)/.rubocop.yml:$CODE_DIR/.rubocop.yml" \ |
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.
Would be nice to share this between the dev-shell setup
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.
Going to try and clean this up in a follow up, with a shared docker-shell
script we can use from here and docker-dev-shell
6300b4d
to
e164687
Compare
RUN pyenv install -s 3.6.13 | ||
RUN pyenv install -s 3.7.1 | ||
RUN pyenv install -s 3.7.10 |
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.
Unrelated to this PR, but I noticed that these still aren't being cached currently. Do you have any idea why that could be?
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.
Not sure, are they being installed again in the tests?
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.
First hunch was some permission issue but I think these, as the tests, are running as root so should be fine.
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.
No, they are being installed in the docker-ci build step in the action actually, when IMO all these layers should be cached?
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.
Oh wait you mean on this PRs build? I busted all the docker images used here so everything needs to rebuild.
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.
Ah yeah I see, I wonder if the change in this PR to use the inline buildkit cache would help with this?
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.
Curious to find out! It might 🤞
@@ -44,6 +44,7 @@ jobs: | |||
echo "BASE_IMAGE=ubuntu:18.04" >> $GITHUB_ENV | |||
echo "CORE_IMAGE=dependabot/dependabot-core" >> $GITHUB_ENV | |||
echo "CORE_CI_IMAGE=dependabot/dependabot-core-ci" >> $GITHUB_ENV |
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.
Went back to using the docker registry as it seemed like ghcr doesn't do well with the inline caching, possibly not storing the cache properly so was re-building every time
run: | | ||
docker push "dependabot/dependabot-core:latest" | ||
- name: Push image to packages (tagged) | ||
docker push "$CORE_IMAGE:latest" |
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.
Changed this to always push the latest
tag, can't see a reason where we don't want to push latest when tagging a release
Use ghcr.io for ci image builds and leve leverage Docker's buildkit caching when building it. Using docker volumes instead of copying all the files over to the ci image. The caching change was previously [reverted in this commit](a89aca9#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f). I believe this will work with [https://github.com/dependabot/dependabot-core/pull/3426](this CI change) as I have vague recalls that the failures happened because we where running all the bundle installs in `Dockerfile.ci`.
09bad5c
to
4aa0b92
Compare
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 think the changes look good and so far the logs here look promising 👍
Leverage Docker's buildkit caching when building
Dockerfile.ci
.Using docker volumes instead of copying all the files over to the ci image as the buildkit cache was previously hanging when building the ci image.
The caching change was previously reverted in this commit.