Docker build #486
Docker build #486
Conversation
| - platform: 'macos-14' | ||
| container: '' | ||
| config: 'release' | ||
| - platform: "macos-14" |
There was a problem hiding this comment.
Those quote changes are automatic from Zed. I can revert them but I don't think it's a big deal.
There was a problem hiding this comment.
Could you please turn off this configuration setting? It's not a big deal, but it'd be nice to not have this happen elsewhere.
| branches: ["main"] | ||
| tags: | ||
| - "v*" | ||
| env: |
There was a problem hiding this comment.
| env: | |
| env: |
| jobs: | ||
| build: | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
Why is this set to false here? To reduce number of iterations when making changes to the CI workflow, in case there are issues on one platform but not another?
.github/workflows/release-docker.yml
Outdated
| - os: ubuntu-22.04-32core-graph-team-amd64 | ||
| platform: linux/amd64 | ||
| binary_name: scip-clang-x86_64-linux | ||
| - os: ubuntu-22.04-32core-graph-team-arm64 | ||
| platform: linux/arm64 | ||
| binary_name: scip-clang-arm64-linux | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
| - os: ubuntu-22.04-32core-graph-team-amd64 | |
| platform: linux/amd64 | |
| binary_name: scip-clang-x86_64-linux | |
| - os: ubuntu-22.04-32core-graph-team-arm64 | |
| platform: linux/arm64 | |
| binary_name: scip-clang-arm64-linux | |
| runs-on: ${{ matrix.os }} | |
| - runner: ubuntu-22.04-32core-graph-team-amd64 | |
| binary-name: scip-clang-x86_64-linux | |
| - runner: ubuntu-22.04-32core-graph-team-arm64 | |
| binary-name: scip-clang-arm64-linux | |
| runs-on: ${{ matrix.runner }} |
Let's use consistent casing conventions for YAML keys. It looks like we're not using the platform keys below; can we remove them?
.github/workflows/release-docker.yml
Outdated
| - uses: bazel-contrib/[email protected] | ||
| with: | ||
| bazelisk-cache: true | ||
| disk-cache: ${{ github.workflow }}-${{ matrix.binary_name }} |
There was a problem hiding this comment.
| disk-cache: ${{ github.workflow }}-${{ matrix.binary_name }} | |
| disk-cache: ${{ github.workflow }}-${{ matrix.binary-name }} |
.github/workflows/release-docker.yml
Outdated
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| path: bazel-bin/indexer/scip-clang | ||
| name: ${{ matrix.binary_name }} |
There was a problem hiding this comment.
| name: ${{ matrix.binary_name }} | |
| name: ${{ matrix.binary-name }} |
.github/workflows/release-docker.yml
Outdated
| - name: Export digest | ||
| run: | | ||
| mkdir -p ${{ runner.temp }}/digests | ||
| digest="${{ steps.build.outputs.digest }}" |
There was a problem hiding this comment.
| digest="${{ steps.build.outputs.digest }}" | |
| digest="${{ steps.build-digest-step.outputs.digest }}" |
| id: build | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: . |
There was a problem hiding this comment.
Would be good to add a comment on why this is necessary. I read the official docs, but couldn't quite follow as to why this argument is necessary.
https://github.com/docker/build-push-action?tab=readme-ov-file#git-context
There was a problem hiding this comment.
By default, this action uses the Git context, so you don't need to use the actions/checkout action to check out the repository as this will be done directly by BuildKit.
As we inject a non-tracked file, we need to explicitly use folder level context (instead of context which is just git ls-files).
I'll add a comment
| type=raw,value=${{ env.PATCH }} | ||
| type=raw,value=${{ env.MINOR }} | ||
| type=raw,value=${{ env.MAJOR }} |
There was a problem hiding this comment.
I don't see these environment variables being set in this workflow file. Are they being set by some other workflow and being injected?
| - name: Create manifest list and push | ||
| working-directory: ${{ runner.temp }}/digests | ||
| run: | | ||
| docker buildx imagetools create $(jq -cr '.tags | map("-t " + .) | join(" ")' <<< "$DOCKER_METADATA_OUTPUT_JSON") \ |
There was a problem hiding this comment.
Would be good to add a comment describing what sets this environment variable; it's not obvious by looking at the configuration.
Could we perhaps split this logic over multiple lines add a comment or two describing the format/conversion with 1 example?
There was a problem hiding this comment.
So this, and several other things that might be confusing come directly from Docker's github actions (most copied verbatim): https://docs.docker.com/build/ci/github-actions/multi-platform/#distribute-build-across-multiple-runners
I will add a comment about the source of those steps, but I consider them internal details of the steps provided by the various docker actions – as such I would rather not deviate textually from the actions they have in their docs – all those env variables are set and processed by their custom actions.
| sha256 = "4b8eff986643b8d9918c4fd3ada9c0eee7e59230a53a46a9bd9686521dcad170", | ||
| urls = ["https://github.com/rhysd/actionlint/releases/download/v1.6.27/actionlint_1.6.27_darwin_arm64.tar.gz"], | ||
| sha256 = "2693315b9093aeacb4ebd91a993fea54fc215057bf0da2659056b4bc033873db", | ||
| urls = ["https://github.com/rhysd/actionlint/releases/download/v1.7.7/actionlint_1.7.7_darwin_arm64.tar.gz"], |
There was a problem hiding this comment.
Could you briefly describe why we needed to bump actionlint here? Did you hit a bug?
There was a problem hiding this comment.
https://github.com/rhysd/actionlint/releases/tag/v1.7.7
Main reason that they didn't support ubuntu-22.04-arm labels – I could add them to actionlint.yaml, but they're built-in, so why not bump it.
Closes GRAPH-1184