-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
refactor: use only goreleaser to build unsigned chainlink images in one workflow #14034
Conversation
* develop: Add finalizer component to TXM (#13638) auto: adjust cron contract imports (#13927) Set PriceMin to match pip-35 definition (#14014) update solana e2e test build deps (#13978) fix data race in syncer/launcher (#14050) [KS-411] Extra validation for FeedIDs in Streams Codec (#14038) [TT-1262] dump pg on failure (#14029) ks-409 fix the mock trigger to ensure events are sent (#14047) update readme's with information about CL node TOML config (#14028) [CCIP-Merge] OCR2 plugins [CCIP-2942] (#14043) [BCF - 3339] - Codec and CR hashed topics support (#14016) common version update to head of develop (#14030)
* develop: [CCIP Merge] Capabilities [CCIP-2943] (#14068) add OZ 5.0.2 contracts (#14065) enable gomods (#14042) auto-10161: replicate v2_3 to v2_3_zksync (#14035) chore: update dependabot config gomod (#14063) Update log trigger default values (#14051) bump solana commit (#14062) Update e2e tests definition for CI and automation workflow (#13908) use services.Config.NewService/Engine (#13851)
if: steps.get-image-tag.outputs.build-publish == 'true' | ||
uses: ./.github/actions/goreleaser-build-sign-publish | ||
with: | ||
enable-docker-publish: "true" |
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.
Did we want this to be false if it was from a PR unless that PR had a specific label?
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 are enabling to publish to the private sdlc ecr. it's a custom thing we created because the regular goreleaser release with snapshot does not do publish at all.
or did you mean we don't want to publish for any pr - just pr with the label build-publish
?
chainlink/.github/actions/goreleaser-build-sign-publish/action_utils
Lines 83 to 86 in 96829b0
if [[ $ENABLE_DOCKER_PUBLISH == "true" ]]; then | |
_publish_snapshot_images | |
_publish_snapshot_manifests | |
fi |
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.
did you mean we don't want to publish for any pr - just pr with the label build-publish
This. Just to minimize the number of images we publish that won't ever get used. But I'm assuming we would use this workflow to build and then optionally publish. Is there still a separate workflow that builds?
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.
ahh I had created the conditional here:
chainlink/.github/workflows/build-publish-develop-pr.yml
Lines 38 to 43 in 96829b0
else | |
if [[ ${{ github.event.label.name }} == 'build-publish' ]]; then | |
echo "image-tag=pr-${{ github.event.number }}-${short_sha}" | tee -a $GITHUB_OUTPUT | |
echo "build-publish=true" | tee -a $GITHUB_OUTPUT | |
fi | |
fi |
so only the label
build-publish
will be built and publishedit starts out with being
false
:echo "build-publish=false" | tee -a $GITHUB_OUTPUT |
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 see. That will skip the build and publish docker job if the label is not on the PR, right? I guess I was assuming that we would have this workflow to cover all normal builds too (PR's without the label) and in those cases, we don't want to publish the image (if the label isn't on the PR).
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 like it if we could hook our E2E tests (which run on almost every PR) into this build step as well and not have our own custom one any more. Is requiring the build-publish
tag necessary for 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.
yeah great point - looking over your ticket I had added this :)
chainlink/.github/workflows/build-publish-develop-pr.yml
Lines 9 to 17 in 37d617b
workflow_dispatch: | |
inputs: | |
git_ref: | |
description: "The git ref to check out" | |
required: true | |
build-publish: | |
description: "Whether to build and publish - defaults to just build" | |
required: false | |
default: "false" |
would this solve your issue?
of course we will need to tweak the
goreleaser-build-sign-publish
composite action a bit
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.
@kalverra that's going to be big (avoid all of those unnecessary builds). Glad you're thinking about that.
We might need to push all images from PRs to ECR though if you're going to use it from tests (unless we used artifacts but...). In that case maybe we have a way to differentiate between docker images from PR's that can be pruned out quickly via lifecycle rules (only used for tests) and docker images from PR's that might need to run in longer 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.
I see QA has different set of secrets for ECR? it pushes to a different repo no?
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.
Right now it does, but I think @kalverra is considering using this image for the tests. If so, we'd need to push all PR images to ECR for your tests, right?
Quality Gate passedIssues Measures |
For a PR to build the image, it needs to have a
build-publish
label attached to the PR.The new image builds will be with the following:
PR builds:
pr-{pr-num}-{sha}
tagTrunk builds:
develop-{sha}
tagRelease branch builds:
release-{sha}
tagManual dispatch builds:
{sha}
tagIt will have a suffix of either
amd64
orarm64
and then there could be another suffix after that with-plugins
that will include plugins in the image.It builds 4 images with the follow tags:
${build_tag}-amd64
${build_tag}-amd64-plugins
${build_tag}-arm64
${build_tag}-arm64-plugins
Tested:
old:
https://github.com/smartcontractkit/chainlink/actions/runs/10309014772?pr=14034
with label trigger:
https://github.com/smartcontractkit/chainlink/actions/runs/10321502972/job/28574553533?pr=14034
with label trigger and matrix split with summary:
https://github.com/smartcontractkit/chainlink/actions/runs/10360943876?pr=14034
with just build - no publish:
https://github.com/smartcontractkit/chainlink/actions/runs/10376300994?pr=14034
Pulled the
arm64
images (with and without plugins) and runs in my local env.Notes:
I tried to use matrix on
os
but it looks like we can't usemacos-latest
runners because docker is not installed on those runners yet:docker/setup-buildx-action#343
https://github.com/orgs/community/discussions/19197
Apparently for the goreleaser split there are two commands to run:
but
continue --merge
does not work for snapshot builds so it errors out and without thecontinue --merge
skip the checksum.txt never gets generated so there is no checksum for the binary.goreleaser/goreleaser#4811