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

build-push-ecr refactor using docker/build-push-action@v3 #28

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thecristen
Copy link
Contributor

@thecristen thecristen commented Jun 20, 2022

This PR tries out rewriting our build-push-ecr workflow to use Docker's actions. Dotcom is using a variation on this in its deployment workflows and it seems to be working okay!

This enables caching. The build-push-action and has many more interesting configuration options that we might want to support, and the metadata-action has more granular tagging abilities; but in this PR I mainly focused on a 1:1 match with our current action's features.

Computes needed docker image tags with metadata-action.

git-$(git rev-parse --short HEAD) --> type=sha,prefix=git- (it defaults to short sha)
additional tag --> type=raw,value=tag
Handles building the image pushing to ECR, enables caching
@thecristen thecristen marked this pull request as ready for review June 20, 2022 21:46
@thecristen thecristen requested a review from paulswartz June 20, 2022 21:47
Copy link
Member

@paulswartz paulswartz left a comment

Choose a reason for hiding this comment

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

This looks good: could you do a branch on mbta/dotcom which uses it so we can make sure it works?

@@ -9,7 +9,7 @@ inputs:
required: true
aws-region:
description: AWS region to use
required: true
required: false
Copy link
Member

Choose a reason for hiding this comment

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

praise: 🤦 thank you for fixing this!

with:
registry: ${{ secrets.docker-repo }}
username: ${{ secrets.aws-access-key-id }}
password: ${{ secrets.aws-secret-access-key }}
Copy link
Member

Choose a reason for hiding this comment

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

praise: this is neat! I didn't know the built-in action could log in directly.

- run: >
for tag in ${{ inputs.docker-additional-tags }}; do
docker tag ${{ steps.docker.outputs.tag }} ${{ inputs.docker-repo }}:$tag
docker push ${{ inputs.docker-repo }}:$tag
echo "type=raw,priority=900,value=${tag},enable=true" >> tags.txt
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: it would be better to write this file into the $RUNNER_TEMP directory. otherwise, any existing content in a tags.txt file will be passed to the job and I don't think that's what you want.

@thecristen
Copy link
Contributor Author

One slight snag / design flaw / opportunity in this:

For Dotcom we run a script against the Docker image, to extract the built assets and deploy them to S3. With the way the docker action works, we end up running docker/build-push-action twice - once with the load: true argument and the second time with the push: true argument. We might want to provide a mechanism for that in our abstraction, maybe by adding load and push as inputs?

@paulswartz
Copy link
Member

@thecristen can you share an example? I would think that you could still be able to extract information from the container even after it was pushed.

@thecristen
Copy link
Contributor Author

@paulswartz Here's an example from Dotcom, where we

  • Build the image (load: true, push: false)
  • Run a script that deploys assets to S3
  • Push the image (load: false, push: true)
  • Deploy to ECS

But upon further reflection my example is not that great, maybe I'm overthinking it and Dotcom can instead

  • This action (pushes the image)
  • Dotcom: use the exported image tag to retrieve image (load: true)
  • Run a script that deploys assets to S3
  • Deploy to ECS

Will report back after trying this refactored action!

@boringcactus
Copy link
Member

GitHub Actions caching makes the Docker build process substantially faster, especially for projects like Glides that smoke test the Docker container in CI and have a lot of Elixir dependencies that change rarely but are slow to compile if not cached. Is this feature likely to land at some point, either by bringing this PR up to date or by making a new PR instead?

@paulswartz
Copy link
Member

@boringcactus if you wanted to take on that work, please feel free!

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