Skip to content
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

Use GitHub Actions for build/pull/tag/push #46

Closed
wants to merge 1 commit into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jun 2, 2022

Description of proposed changes

This replaces the existing scripts with GitHub Actions provided by Docker.

Also has the benefit of using buildx which supports building multi-arch images (to be done separately in #48): https://docs.docker.com/desktop/multi-arch/

Sequence of steps inspired by https://github.com/docker/build-push-action/blob/master/docs/advanced/isolated-builders.md.

Related issue(s)

Testing

See CI runs. However, this doesn't test the execution path for push trigger on the default branch master.

@victorlin victorlin self-assigned this Jun 2, 2022
@victorlin victorlin force-pushed the victorlin/use-buildx branch from f824de5 to f891905 Compare June 2, 2022 22:45
Comment on lines +64 to +70
# The nextstrain/base Dockerfile is a multi-stage with both a "builder" target
# and a main target. To enable proper caching via --cache-from we need both
# these images available to pull layers from. This means pulling both in at
# the start and pushing both up at the end.

# Calling `docker run nextstrain/base` will still only pull down the small base
# image rather than pulling down the larger nextstrain/base-builder image.
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if isolated builders are needed here though. It might be more efficient to have one build and push to both base and builder images, if that's even possible?

Or, to cut some time, build the two images in parallel using 2 jobs in this workflow.

Copy link
Member

Choose a reason for hiding this comment

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

I believe one of the features of buildx is better support for multi-stage builds, such that you can do just build operation for a multi-stage build but still push up tagged images for each stage (instead of having to explicitly build each stage yourself just so you can push each later).

I'm not sure how building both stages in parallel would work since the second stage relies on the first?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this better support appears to be (1, 2) in the form of exporting cached layers via type=registry with mode=max (meaning "all intermediate layers"), but you have to use a separate image tag/name (unlike type=inline, but that only supports mode=min which means "final layers").

This replaces the existing scripts with GitHub Actions provided by Docker.

Also has the benefit of using buildx which supports building multi-arch images (to be done later): https://docs.docker.com/desktop/multi-arch/

Sequence of steps inspired by https://github.com/docker/build-push-action/blob/master/docs/advanced/isolated-builders.md.
@victorlin victorlin force-pushed the victorlin/use-buildx branch from f891905 to 0b4fb2d Compare June 6, 2022 21:41
@victorlin victorlin marked this pull request as ready for review June 6, 2022 22:13
@victorlin victorlin requested a review from tsibley June 6, 2022 22:13
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

+1 for using buildx here, but as-is I'm not sure this seems like an improvement to me over the modular shell scripts. In particular, I think the GitHub Actions flow is much harder to follow, and it seems unfortunate that there's no longer a shared toolchain for building the images locally.

I wonder about instead ditching build-push-action here but keeping setup-buildx and then keeping the modular programs in devel/ but extending them to use docker buildx build … and pass --builder … and --platforms …. (Alternatively, I think docker buildx build could even be made optional by setting install: true for setup-buildx, but not sure that's a good idea or not.)

Comment on lines +26 to +27
echo "CACHE_DATE=$(date --utc +%Y%m%dT%H%M%SZ)" >> $GITHUB_ENV
echo "GIT_REVISION=$(git describe --tags --abbrev=40 --always --dirty || true)" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

I like to do | tee -a instead of >> so that the values are visible in the workflow logs.

Comment on lines +29 to +56
# Use toJSON to store boolean values in a string
# and fromJSON to retrieve as boolean later on.
- name: Set common variables
run: |
echo "BASE_IMAGE_NAME=nextstrain/base" >> $GITHUB_ENV
echo "BUILDER_IMAGE_NAME=nextstrain/base-builder" >> $GITHUB_ENV
echo "TAG_AND_PUSH=${{ toJSON(github.event_name != 'pull_request') }}" >> $GITHUB_ENV
echo "ON_DEFAULT_BRANCH=${{ toJSON(github.ref_type == 'branch' && github.ref_name == github.event.repository.default_branch) }}" >> $GITHUB_ENV

- if: ${{ fromJSON(env.TAG_AND_PUSH) && fromJSON(env.ON_DEFAULT_BRANCH) }}
name: Set tag name (default branch)
run: echo "TAG_NAME=build-$CACHE_DATE" >> $GITHUB_ENV

# From `man docker-image-tag`: The tag name must be valid ASCII and may
# contain lowercase and uppercase letters, digits, underscores, periods
# and hyphens.
# TODO: parameterize this as workflow input
- if: ${{ fromJSON(env.TAG_AND_PUSH) && !fromJSON(env.ON_DEFAULT_BRANCH) }}
name: Set tag name (non-default branch)
run: echo "TAG_NAME=branch-${GITHUB_REF_NAME//[^A-Za-z0-9._-]/-}" >> $GITHUB_ENV

- if: ${{ fromJSON(env.TAG_AND_PUSH) }}
name: Set tags to push
run: |
maybe_base_image_latest_tag=${{ fromJSON(env.ON_DEFAULT_BRANCH) && ',$BASE_IMAGE_NAME:latest' || '' }}
echo "BASE_IMAGE_TAGS=$BASE_IMAGE_NAME:$TAG_NAME$maybe_base_image_latest_tag" >> $GITHUB_ENV
maybe_builder_image_latest_tag=${{ fromJSON(env.ON_DEFAULT_BRANCH) && ',$BUILDER_IMAGE_NAME:latest' || '' }}
echo "BUILDER_IMAGE_TAGS=$BUILDER_IMAGE_NAME:$TAG_NAME$maybe_builder_image_latest_tag" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

This is all pretty dense and hard for me to follow given the mix of interpolation at different levels, JSON serde, and conditionals. The motivation for defining and using all these is because the build-push-actions needed to be parameterized instead of themselves being in a conditional like with the modular devel/tag + devel/push commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I couldn't figure out a better way since GitHub Actions provides limited support for conditional "code paths" here. It would be either some variant of this (with comments to explain) or several copies of build-push-action that are mostly the same except for push, tags, and step condition.

Comment on lines +81 to +83
cache-from: |
type=registry,ref=${{ env.BUILDER_IMAGE_NAME }}
type=registry,ref=${{ env.BASE_IMAGE_NAME }}
Copy link
Member

Choose a reason for hiding this comment

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

The lack of cache-to here might mean that the subsequent step can't use the just-built first stage when TAG_AND_PUSH is false (and would have to re-download it with TAG_AND_PUSH is true).

Comment on lines +97 to +99
cache-from: |
type=registry,ref=${{ env.BUILDER_IMAGE_NAME }}
type=registry,ref=${{ env.BASE_IMAGE_NAME }}
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, the lack of non-registry sources in this cache-from here might mean that this step can't use the just-built first stage from the previous step when TAG_AND_PUSH is false (and would have to re-download it with TAG_AND_PUSH is true).

Comment on lines +64 to +70
# The nextstrain/base Dockerfile is a multi-stage with both a "builder" target
# and a main target. To enable proper caching via --cache-from we need both
# these images available to pull layers from. This means pulling both in at
# the start and pushing both up at the end.

# Calling `docker run nextstrain/base` will still only pull down the small base
# image rather than pulling down the larger nextstrain/base-builder image.
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this better support appears to be (1, 2) in the form of exporting cached layers via type=registry with mode=max (meaning "all intermediate layers"), but you have to use a separate image tag/name (unlike type=inline, but that only supports mode=min which means "final layers").

@victorlin
Copy link
Member Author

@tsibley: I wonder about instead ditching build-push-action here but keeping setup-buildx and then keeping the modular programs in devel/ but extending them to use docker buildx build …

That sounds reasonable, it addresses the motivation for this with much less of a change.

@victorlin victorlin mentioned this pull request Jun 14, 2022
@victorlin
Copy link
Member Author

Closing in favor of #49.

@victorlin victorlin closed this Jul 7, 2022
@victorlin victorlin deleted the victorlin/use-buildx branch July 7, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants