-
Notifications
You must be signed in to change notification settings - Fork 43
ci: build and push image for main
branch
#222
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
Conversation
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.
self review
./scripts/build.sh \ | ||
--arch=amd64 \ | ||
--base=$BASE \ | ||
--tag=$VERSION |
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 am not building a multi-arch here, as this step only ensures that Dockerfile builds 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.
we should probably still build multi-arch as some changes may only cause issues with certain architectures
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.
Ok. Our build script uses --load
when --push
is not specified, which cannot be used for multi arch images.
docker/buildx#59
I didn't look deep if we actually needed to load the built images during local development otherwise I can just remove --load
and build a multi arch image.
While calling the script locally we only build with $current
arch so --load
works fine.
@@ -63,7 +62,7 @@ else | |||
args+=( --load ) | |||
fi | |||
|
|||
docker buildx build "${args[@]}" -t $base:$tag -t $base:latest -f Dockerfile . | |||
docker buildx build --builder $BUILDER_NAME "${args[@]}" -t $base:$tag -t $base:latest -f Dockerfile . |
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.
Instead of setting the builder as a default builder, use it explicitly to avoid changing the developer's local builder configuration.
@@ -63,7 +62,7 @@ else | |||
args+=( --load ) | |||
fi | |||
|
|||
docker buildx build "${args[@]}" -t $base:$tag -t $base:latest -f Dockerfile . | |||
docker buildx build --builder $BUILDER_NAME "${args[@]}" -t $base:$tag -t $base:latest -f Dockerfile . |
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.
could also tag as $base:main
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 thought about this but will require hacking the build script to work differently for tagged releases. Currently the script has a hardcoded $base:latest
so I opted for changing the $base
.
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.
fair, not blocking
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.
LGTM
To what extent has this workflow been tested?
Has the workflow been linted?
# Needed to get older tags | ||
fetch-depth: 0 |
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 probably don't need the tags if we're just building a preview image on main
?
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. The benefit of the version
tag is to use any previous image. It's a choice we should make. If we only tag with main
then we will only have a single tagged image pointing to the tip of the main
.
@dannykopping I have tested the workflow except the push image step, but will pass this through the linter. Thanks for calling it. |
@matifali is this good to merge? |
Yes. Once the CI passes it should be good to merge. |
This pull request updates the
ci
workflow to build and push a Docker image for themain
branch.Image is pushed to
ghcr.io/coder/envbuilder-preview
to not pollute the manually tagged versions atghcr.io/coder/envbuilder
This will help test the features that are under development.
Each image is tagged as
latest
and$(./scripts/version.sh)-dev-$(git rev-parse --short HEAD)
e.g.,ghcr.io/coder/envbuilder-preview:latest
ghcr.io/coder/envbuilder-preview:v0.2.9-dev-ab86184
Alternatively, we can push
ghcr.io/coder/envbuilder:main
to the same package.