-
Notifications
You must be signed in to change notification settings - Fork 215
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(docker): use Depot for multiarch images #10039
Conversation
8817440
to
a083021
Compare
Deploying agoric-sdk with Cloudflare Pages
|
a083021
to
02aa9e2
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.
This one is flying a bit above my head. One thing however, I don't see an equivalent PR in agoric-3-proposals
for the patch here.
82c3e3f
to
e7fb573
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.
So far this seems overly complex to me. Let's figure out how to simplify.
- fromTag: "latest" | ||
+ fromTag: "latest", | ||
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:", | ||
+ sdkRepositoryColon: "ghcr.io/agoric/agoric-sdk:", |
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 is this value changed?
or repositoryColon
for that matter?
// Must match the Bake file and CI config | ||
- fromTag: "latest" | ||
+ fromTag: "latest", | ||
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:", |
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 is the colon necessary to include?
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.
given there are multiple repositories in play, both git and Docker, please give this a more descriptive name. assuming we can drop the colon,
+ repositoryColon: "ghcr.io/agoric/agoric-3-proposals:", | |
+ a3pDockerRepository: "ghcr.io/agoric/agoric-3-proposals", |
RESUME(fromTag) { | ||
return ` | ||
## RESUME | ||
-FROM ghcr.io/agoric/agoric-3-proposals:${fromTag} as use-${fromTag} |
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 are these Docker vars necessary? the Dockerfile is generated by the CI job. Couldn't it just produce the desired file contents?
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.
The buildConfig
is part of the package.json
. Would you prefer that CI edited that before prepare-build instead of creating a docker-bake.override.json
to modify the Dockerfile's behaviour at build time?
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.
yes, very much
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've removed any dependencies on a synthetic-chain patch, and reduced the postprocessing of the prepare-build
-generated files to a single sed
. Let's talk about the best way to make that change robust.
62d8aaf
to
d901bda
Compare
a3p-integration/package.json
Outdated
@@ -12,7 +12,7 @@ | |||
"doctor": "yarn synthetic-chain doctor" | |||
}, | |||
"dependencies": { | |||
"@agoric/synthetic-chain": "patch:@agoric/synthetic-chain@npm%3A0.1.0#~/.yarn/patches/@agoric-synthetic-chain-npm-0.1.0-148de716a6.patch", | |||
"@agoric/synthetic-chain": "^0.1.1", |
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.
might as well bump to 0.3.0
d901bda
to
5820004
Compare
I extracted the controversial "publish |
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 don't feel knowledgeable enough with the depot stuff to approve, but from what I'm seeing this look fine to me.
- name: Save BUILD_TAG | ||
run: | | ||
ARCH=$(echo '${{ matrix.platform }}' | tr / _) | ||
echo "BUILD_TAG=${{ needs.snapshot.outputs.tag }}-$ARCH" >> $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.
Are we still generating -$ARCH
tags? I have used those in the past, but mostly because the arm64 and thus combined image took forever to publish, so possibly this is no longer necessary.
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.
You're right; building and publishing the combined image now takes about the same time as the -linux_amd64
tag used to take. So, no more -$ARCH
tags.
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.
Nice improvement!
LGTM save for moving :
into the variable. I trust you'll address before merging
.github/workflows/docker.yml
Outdated
PREFIX="$REGISTRY/agoric/cosmic-swingset-solo:" | ||
for TAG in ${{ needs.docker-sdk.outputs.tags }}; do | ||
PREFIXED="$PREFIXED$sep$IMAGE:$TAG" | ||
PREFIXED="$PREFIXED$sep$PREFIX$TAG" |
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 this change? the previous was more clear. please revert or justify.
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.
Okay.
5820004
to
91d2990
Compare
Project ID file accidentally missing from #10039 which caused the following error in CI: ``` Error: could not configure buildx: unknown project ID (run `depot init` or use --project or $DEPOT_PROJECT_ID) ``` This was confirmed fixed with a manual run of `Docker Build Release Images`.
closes: #9043
Description
Update
docker.yml
image publishing to use Depot for efficient multiarch builds.Before (~2h):
After (~15m):
Also, stop building the
agoric/agoric-sdk:ibc-alpha
tag since it is no longer relevant (only was needed by an IBC test long, long ago).Security Considerations
Introduces CI dependency on Depot.
agoric-3-proposals
already uses this successfully, so it is not new to our organisation.Scaling Considerations
Improves scalability of our CI system (2h Docker multiarch build times are reduced to 15m).
Documentation Considerations
n/a
Testing Considerations
Manually launched Docker builds show that the new CI works correctly.
Upgrade Considerations
n/a