Skip to content

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

Merged
merged 7 commits into from
Jun 10, 2024
Merged

ci: build and push image for main branch #222

merged 7 commits into from
Jun 10, 2024

Conversation

matifali
Copy link
Member

@matifali matifali commented Jun 5, 2024

This pull request updates the ci workflow to build and push a Docker image for the main branch.

Image is pushed to ghcr.io/coder/envbuilder-preview to not pollute the manually tagged versions at ghcr.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.,

  1. ghcr.io/coder/envbuilder-preview:latest
  2. ghcr.io/coder/envbuilder-preview:v0.2.9-dev-ab86184

Alternatively, we can push ghcr.io/coder/envbuilder:main to the same package.

Copy link
Member Author

@matifali matifali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review

Comment on lines +99 to +102
./scripts/build.sh \
--arch=amd64 \
--base=$BASE \
--tag=$VERSION
Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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 .
Copy link
Member Author

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.

@matifali matifali requested a review from johnstcn June 5, 2024 14:56
@matifali matifali marked this pull request as ready for review June 5, 2024 14:58
@johnstcn johnstcn requested review from dannykopping and mtojek June 6, 2024 08:26
@@ -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 .
Copy link
Member

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

Copy link
Member Author

@matifali matifali Jun 6, 2024

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair, not blocking

Copy link
Contributor

@dannykopping dannykopping left a 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?

Comment on lines +77 to +78
# Needed to get older tags
fetch-depth: 0
Copy link
Member

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?

Copy link
Member Author

@matifali matifali Jun 6, 2024

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.

@matifali
Copy link
Member Author

matifali commented Jun 6, 2024

@dannykopping I have tested the workflow except the push image step, but will pass this through the linter. Thanks for calling it.

@johnstcn
Copy link
Member

@matifali is this good to merge?

@matifali
Copy link
Member Author

Yes. Once the CI passes it should be good to merge.

@matifali matifali merged commit df6068e into main Jun 10, 2024
4 checks passed
@matifali matifali deleted the build-main branch June 10, 2024 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants