buildkite-plugins / docker-compose-buildkite-plugin

🐳⚡️ Run build scripts, and build + push images, w/ Docker Compose
MIT License
172 stars 139 forks source link

By default build will also push the image to remote repository #380

Closed jamesone closed 7 months ago

jamesone commented 1 year ago

Is this meant to be the default behaviour? I expect it only to push if I define the push configuration?

Here is my config:

- docker-compose#v4.11.0:
          mount-buildkite-agent: true
          image-repository: REMOTE_REPO
          image-name: base_image_branch-${FRIENDLY_BRANCH_NAME}
          build: base_image
          args:
            - NPM_TOKEN
          environment:
            - FRIENDLY_BRANCH_NAME
          cli-version: 2.2
          cache-from:
            - base_image: REMOTE_REPO:base_image_branch-${FRIENDLY_BRANCH_NAME}
          propagate-environment: true
          config:
            - docker-compose.yml

After building successfully, it goes ahead and pushes.

:docker: Pushing built images to REMOTE_REPO

I would expect it only to push if the push array is present, unless I'm missing something?

I looked at this merged PR: https://github.com/buildkite-plugins/docker-compose-buildkite-plugin/pull/224/files - and the code suggests that it will only push, if the push array present. Main branch still contains this same logic

toote commented 1 year ago

This is indeed the current behaviour and I agree that it is confusing and counter-intuitive.

Similar things also occur in other situations. For example:

We are currently thinking of changes to simplify, clarify and unify those behaviours. As it is a huge change, it will be backwards incompatible and be a major release of this plugin. We hope to have something - if time allows - in the next few months

jamesone commented 1 year ago

@toote thanks for the update.

RE "This is indeed the current behaviour and I agree that it is confusing and counter-intuitive."

Should we change this behaviour? I don't suspect this is a "huge" change, however, I don't know the repo. Or is this a big change and would be apart of the next major release.

toote commented 1 year ago

@jamesone feel free to open a PR with whatever changes you see fit. We will be more than happy to review it and iterate on it as necessary

toote commented 7 months ago

This change will be part of the next major release that will include #416 and #418