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

Composite Docker action #208

Open
Brutus5000 opened this issue Oct 27, 2020 · 13 comments · May be fixed by #453
Open

Composite Docker action #208

Brutus5000 opened this issue Oct 27, 2020 · 13 comments · May be fixed by #453

Comments

@Brutus5000
Copy link

Hi guys,

this is not really an issue, but some feedback that you hopefully consider:
I'm aware I probably sound like a whiny crybaby, but please believe me that I admire your efforts... but seriously: with v2 you raped a good working Github action.

And yes yes, you're probably thinking: Why don't you fix it then yourself? But I'm already managing two dozen repositories for the FAForever project and I just need an easy and working CI infrastructure. [And v1 is still available, so technically no problem.]

Why do I use this action?
I prefer Github actions because they are easier to read and simplified building blocks over writing it in a shell script, even if they are a few (!) lines longer

And v1 of this action is the perfect solution for the workflow I need (with some Github if expression magic):

      - name: Build and push Docker images
        if: github.ref == 'refs/heads/develop' || startsWith(github.ref, 'refs/tags')
        uses: docker/build-push-action@v1.1.1
        with:
          username: ${{ secrets.DOCKER_USERNAME }}
          password: ${{ secrets.DOCKER_PASSWORD }}
          repository: myproject/myrepo
          tag_with_ref: true

What has changed?
With v2 I need to split it off in 3 steps (+ an additional one considering #100):

  • one step declaring the tag I want to push
  • one step for a login (yes, if I want to push I ALWAYS want to login)
  • one step for the build & push

Eventually I end up with 3x the lines of code that I had before. No thanks, I rather stick with the previous version or go back to manual shell commands.

Here are some ideas to improve on v2:

  • I do understand that it makes sense to have a login action if you want to pull images from a protected repository. But why does this have to affect the build-and-push step? If already logged in for other reason, this is the special case not the default, so my proposal would be to either ignore this case (double login then) or to offer an login opt-out config flag instead.
  • I do understand that the tag_with_ref flag was quirky and limits some scenarios like pushing multiple tags and I actually like that there is another way now to configure this now. But was it necessary to drop the existing flag? Maybe you can consider bringing it back under a more meaningful name.
  • Looking at the upgrade guide it seems that context and file are mandatory now (didn't test it actually) but then again: why would I need to declare what is the 99% default case? So if it's not mandatory I'd to remove it from the upgrade document, and if it is mandatory I'd suggest to offer default settings for a classic Dockerfile build

I'd love to see a very powerful action as it is now, with sensible default settings preconfigured for the simple use cases.

Love you guys! Keep up the good work! ❤️

@crazy-max
Copy link
Member

crazy-max commented Oct 28, 2020

Hi @Brutus5000, thanks for your feedback!

I do understand that it makes sense to have a login action if you want to pull images from a protected repository. But why does this have to affect the build-and-push step? If already logged in for other reason, this is the special case not the default, so my proposal would be to either ignore this case (double login then) or to offer an login opt-out config flag instead.

Actually it makes sense to have a dedicated login action because you can define tags affecting multiple registries which was not possible in v1. I'll give you an example with an image that will be published on both Docker Hub and GHCR:

# v2
steps:
  -
    name: Checkout
    uses: actions/checkout@v2
  -
    name: Set up Docker Buildx
    uses: docker/setup-buildx-action@v1
  -
    name: Login to DockerHub
    uses: docker/login-action@v1
    with:
      username: ${{ secrets.DOCKERHUB_USERNAME }}
      password: ${{ secrets.DOCKERHUB_TOKEN }}
 -
    name: Login to GitHub Container Registry
    uses: docker/login-action@v1
    with:
      registry: ghcr.io
      username: ${{ github.repository_owner }}
      password: ${{ secrets.CR_PAT }} 
 -
    name: Build and push
    uses: docker/build-push-action@v2
    with:
      context: .
      push: true
      tags: |
        user/app:latest
        ghcr.io/${{ github.repository_owner }}/app:latest
# v1
steps:
  -
    name: Checkout code
    uses: actions/checkout@v2
  -
    name: Build and push to Docker Hub
    uses: docker/build-push-action@v1
    with:
      username: ${{ secrets.DOCKERHUB_USERNAME }}
      password: ${{ secrets.DOCKERHUB_TOKEN }}
      repository: user/app
      tags: latest
  -
    name: Build and push to GHCR
    uses: docker/build-push-action@v1
    with:
      username: ${{ github.repository_owner }}
      password: ${{ secrets.CR_PAT }} 
      repository: ghcr.io/${{ github.repository_owner }}/app
      tags: latest

As you can see here you must duplicate as many times the number of registries as you wish to use for v1. So you don't have as much flexibility on the use of tags and also build will be done x times the number of registries. The fact of having a dedicated action for the connection also allows to reuse it in other actions that need to connect to a registry.

And finally, a significant point concerns certain registries that use specific authentication methods through their own action. This is for example the case for AWS:

name: ci

on:
  push:
    branches: master

jobs:
  login:
    runs-on: ubuntu-latest
    steps:
      -
        name: Configure AWS Credentials
        uses: aws-actions/configure-aws-credentials@v1
        with:
          aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}
          aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
          aws-region: <region>
      -
        name: Login to ECR
        uses: docker/login-action@v1
        with:
          registry: <aws-account-number>.dkr.ecr.<region>.amazonaws.com

I do understand that the tag_with_ref flag was quirky and limits some scenarios like pushing multiple tags and I actually like that there is another way now to configure this now. But was it necessary to drop the existing flag? Maybe you can consider bringing it back under a more meaningful name.

I'm currently working on an action called Docker meta allowing a management of tags and labels similar to the old flags used in v1. #206 was created to integrate this action as an example.

Looking at the upgrade guide it seems that context and file are mandatory now (didn't test it actually) but then again: why would I need to declare what is the 99% default case? So if it's not mandatory I'd to remove it from the upgrade document, and if it is mandatory I'd suggest to offer default settings for a classic Dockerfile build

Context and file are not mandatory. The default Git context is used if no context is defined. This allows you to stop using the step actions/checkout. This has been added in the upgrade notes for an easy and reproducible transition for users coming from v1.

@crazy-max
Copy link
Member

crazy-max commented Oct 28, 2020

@Brutus5000

But I'm already managing two dozen repositories for the FAForever project and I just need an easy and working CI infrastructure.

About this I think a Composite action would fit perfectly for you. But atm it's not possible to use actions within actions with composite (see actions/runner#646). What you can do is sharing workflows with your organization.

@worldofgeese
Copy link

worldofgeese commented Oct 28, 2020

I'm currently working on an action called Docker meta allowing a management of tags and labels similar to the old flags used in v1. #206 was created to integrate this action as an example.

But this is one of the many issues with v2: you keep making new actions (more complexity) when what users want is something simple out-of-the-box with optional complexity. And even those options should be presented simply.

As an example: why am I leveraging multiple caches with opaque language? It's starting to look like a DSL. A key-value of cache: yes should be all that is needed. This

          cache-from: type=registry,ref=user/app:latest
          cache-to: type=inline

and

          cache-from: type=local,src=/tmp/.buildx-cache
          cache-to: type=local,dest=/tmp/.buildx-cache

mean nothing to the uninitiated. If that is the language used behind the scenes, don't present that to the user!

You've taken something that needed a few tweaks and turned it into something for people to spend hours pouring over documentation for the right recipe of incantations that result in a successful build.

@Brutus5000
Copy link
Author

Brutus5000 commented Oct 28, 2020

@crazy-max Thanks for your suggestions I will look into them.

Regarding the other points:
As I said, I do get the point of a dedicated login step. However, for me the question is: Do we need to drop the login in the build-and-push just because there is a dedicated login step now? Why not make it dependent on the presence or absence of the username and password env variables? Pushing to more than one docker registry sounds like a very rare edge case to me. It's good to support it (I guess especially to support your enterprise customers), but why force the 99% of the people who just live with one registry to split up login and push? Is there any drawback with offering both ways for login?

@alerque
Copy link

alerque commented Nov 18, 2020

I understand there are edge cases where you want more controls, but v2 is a serious regression in usability over v1 ... to the point where I've been holding updates and even downgrading some to get the simple reliable easy to configure v1 back. If this keeps going this way I'll be looking for alternative actions altogether because it's just silly to have such complex workflows with multiple things to bump for such simple integrated tasks. As the OP noted, the advanced features should have been added while keeping sensible defaults in place for simpler use cases.

@crazy-max
Copy link
Member

crazy-max commented Nov 18, 2020

@worldofgeese @Brutus5000 @alerque

A single (or more concise) step as it was the case for v1 will be possible when GitHub will have implemented nested actions through a composite action (actions/runner#646). Here is an example of a composite action named for example crazy-max/docker-action@v1:

# https://github.com/crazy-max/docker-action/blob/v1/action.yml
name: 'My Docker action'
description: 'Docker AIO action'
author: 'crazy-max'

inputs:
  registry:
    description: 'Server address of the Docker registry'
    required: false
  image:
    description: 'Docker image'
    required: true
  username:
    description: 'Username to log in to a Docker registry'
    required: false
  password:
    description: 'Password or PAT to log in to a Docker registry'
    required: false
  push:
    description: 'Push'
    required: false
    default: 'false'

runs:
  using: 'composite'
  steps:
    -
      name: Checkout
      uses: actions/checkout@v2
    -
      name: Docker meta
      id: docker_meta
      uses: crazy-max/ghaction-docker-meta@v1
      with:
        images: ${{ inputs.image }}
    -
      name: Set up QEMU
      uses: docker/setup-qemu-action@v1
    -
      name: Set up Docker Buildx
      uses: docker/setup-buildx-action@v1
    -
      name: Login to DockerHub
      if: github.event_name != 'pull_request' && inputs.username != '' && inputs.password != ''
      uses: docker/login-action@v1
      with:
        username: ${{ inputs.username }}
        password: ${{ inputs.password }}
   -
      name: Build and push
      uses: docker/build-push-action@v2
      with:
        context: .
        push: ${{ inputs.push }}
        tags: ${{ steps.docker_meta.outputs.tags }}
        labels: ${{ steps.docker_meta.outputs.labels }}

And a possible workflow to use it:

name: ci

on:
  push:

jobs:
  ci:
    runs-on: ubuntu-latest
    steps:
      -
        name: Docker build
        uses: crazy-max/docker-action@v1
        with:
          registry: ghcr.io # optional for Docker Hub
          image: name/app
          username: ${{ secrets.GHCR_USERNAME }}
          password: ${{ secrets.GHCR_PAT }}
          push: ${{ github.event_name != 'pull_request' }}

@alerque
Copy link

alerque commented Nov 19, 2020

@CrazyMax I'll look forward to a v3 of this action (or a new one entirely) that works like that and brings some sanity to simpler projects. For now v1 still functions nicely. 😉

@ianfixes
Copy link

ianfixes commented Dec 7, 2020

I'll look forward to a v3 of this action (or a new one entirely) that works like that and brings some sanity to simpler projects. For now v1 still functions nicely.

Is there an issue open specifically to track that?

@crazy-max
Copy link
Member

@ianfixes Upstream issue actually actions/runner#646.

@ianfixes
Copy link

ianfixes commented Dec 7, 2020

Thanks. I think my question was unclear though.

You described crazy-max/docker-action@v1 as a way of using actions/runner#646 to create a composite action in your own repo. I'm asking whether this repo (or the docker org in general) will host such an action when 646 is completed.

@crazy-max crazy-max changed the title Feedback for version 2 Composite Docker action Feb 16, 2021
alerque added a commit to alerque/casile that referenced this issue Mar 16, 2021
The official GH Action for this gets worse and worse with every version,
and the old versions aren't properly supported any more.

cf. docker/build-push-action#208

This isn't actually that hard, and with a make rule for it we have the
option to build and push locally.
alerque added a commit to sile-typesetter/casile that referenced this issue Mar 16, 2021
The official GH Action for this gets worse and worse with every version,
and the old versions aren't properly supported any more.

cf. docker/build-push-action#208

This isn't actually that hard, and with a make rule for it we have the
option to build and push locally.
@crazy-max crazy-max linked a pull request Aug 30, 2021 that will close this issue
@mattwelke
Copy link

mattwelke commented Oct 16, 2021

Thanks. I think my question was unclear though.

You described crazy-max/docker-action@v1 as a way of using actions/runner#646 to create a composite action in your own repo. I'm asking whether this repo (or the docker org in general) will host such an action when 646 is completed.

It might be a good idea for the community to put together a "docker-build-push-simple-action" as a stopgap while waiting for actions/runner#646. Once that's complete, if Docker decides to create a new action for simple use cases, the community-maintained stopgap could be deprecated in favor of the new Docker-maintained solution. If Docker decides not to create such an action, the community-maintained action could be maintained as long as demand for it existed.

@benfrisbie
Copy link

Composite actions have supported runs.steps[*].uses since late 2021. I didn't see any official support for a composite action yet, so I put one together at https://github.com/benfrisbie/docker-tag-build-push-action.

It doesn't support all of the inputs and outputs from docker/login-action, docker/metadata-action, and docker/build-push-action. However, it supports most of the main ones that I needed for my personal use. I plan on adding support for the rest of them soon.

Anyways, I'd welcome some feedback on it! If there is interest to get it moved into the docker organization I'd love to discuss that as well.

@crazy-max
Copy link
Member

@benfrisbie #453

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 a pull request may close this issue.

7 participants