redhat-actions / push-to-registry

GitHub Action to push a container image to an image registry.
https://github.com/marketplace/actions/push-to-registry
MIT License
100 stars 33 forks source link

Refactor inputs to support docker/metadata-action #50

Closed ntkme closed 3 years ago

ntkme commented 3 years ago

This PR adds support for docker/metadata-action@v2. See https://github.com/redhat-actions/buildah-build/issues/74 and https://github.com/redhat-actions/buildah-build/pull/76.

Description

Support tags input in full image name form. When full image name form is used, input image and input registry would be ignored.

Related Issue(s)

https://github.com/redhat-actions/buildah-build/issues/74 https://github.com/redhat-actions/buildah-build/pull/76

Checklist

Changes made

Support docker/metadata-action

tetchel commented 3 years ago

Specifically, this PR introduces the concept of "full name tags", so we have to distinguish between the registry/image/tag use-cases (the old way) and the image-only use-case (the new way).

It should be explained in the Inputs section and possibly in its own section if it is too complex to fit nicely there.

We can also make this change ourselves, if you prefer.

edit: this applies to the buildah PR too

tetchel commented 3 years ago

It would be great if we could capture this imageWithTag logic into one location, though I understand you are just working with the way it was written already. But the way it is written, it's hard to know if we found all the spots the logic has to change.

Maybe the logic should be lifted up to the entrypoint function and passed down through a parameter.

image

ntkme commented 3 years ago

We can also make this change ourselves, if you prefer.

@tetchel Please feel free to push documentation changes to the working branch. I have edit permission enabled for maintainers.

The buildah-build action PR should be good to go in terms of functionality, while I'm working on fixing pulling from docker in push-to-registry action (pulling from docker-daemon may overwrite image in podman with same name, thus need to pull with different --root and --stroage-opt).

ntkme commented 3 years ago

The issue with having same full image name tag on both docker and podman has been fixed by introducing a temporary storage --root. The --storage-opts' logic is copied from buildah action.

Looking at the action run on my fork, it appears everything now function correctly except that my fork failed to push images due to lack of credentials.

ntkme commented 3 years ago

During the test I found a bug where occasionally the Created timestamp from podman/docker images becomes NaN. It is due to https://github.com/actions/toolkit/pull/773, so that I updated @actions/exec to latest.

Here is an example of this bug: https://github.com/redhat-actions/push-to-registry/runs/3833643212?check_suite_focus=true

In the screenshot you can see that supposedly podman's timestamp is later, but the output is saying docker's timestamp is later:

Screenshot 2021-10-08 12 02 24 AM

divyansh42 commented 3 years ago

During the test I found a bug where occasionally the Created timestamp from podman/docker images becomes NaN. It is due to actions/toolkit#773, so that I updated @actions/exec to latest.

Here is an example of this bug: https://github.com/redhat-actions/push-to-registry/runs/3833643212?check_suite_focus=true

In the screenshot you can see that supposedly podman's timestamp is later, but the output is saying docker's timestamp is later:

Screenshot 2021-10-08 12 02 24 AM

Thanks for bringing this up. Just now I tested this behaviour, I have updated the @actions/exec to the latest version but I am still getting NaN. Don't know if they have tried to fix that in latest version. Here is the workflow run with the updated dependency: https://github.com/redhat-actions/push-to-registry/runs/3837698262?check_suite_focus=true#step:6:113

ntkme commented 3 years ago

@divyansh42 Looking at the commit history for test branch, dist/index.js is still compiled with the old version. Also, the bundle check did not pass: https://github.com/redhat-actions/push-to-registry/runs/3837819085?check_suite_focus=true

I think you probably forgot the pre-commit hook?

divyansh42 commented 3 years ago

@divyansh42 Looking at the commit history for test branch, dist/index.js is still compiled with the old version. Also, the bundle check did not pass: https://github.com/redhat-actions/push-to-registry/runs/3837819085?check_suite_focus=true

I think you probably forgot the pre-commit hook?

yeah, nvm I have done something wrong. Just figuring that out.

Edit: Verified this again and it's working fine. Thanks for fixing this.

divyansh42 commented 3 years ago

Can we combine workflows Multiple containers CLI build tests and Multiple containers CLI build tests with full image name into one file by using a matrix? As the only difference in these two workflows is input tags

ntkme commented 3 years ago

@divyansh42 Made some big change to the matrix build. Took a while for me to get the syntax correct, but now it uses one job definition to generate all 16 jobs in the matrix. You can see it here: https://github.com/ntkme/push-to-registry/actions/runs/1321168922 (all push failed because quay.io credentials does not exist on my fork).

ntkme commented 3 years ago

@divyansh42 Commit 213d6c4 failed the login test because podman system reset -f cleared auth file created by login action so that the post logout part failed. Now replaced with podman rmi -a -f, which should not have such side effect.

divyansh42 commented 3 years ago

podman system reset was a bad idea as it cleared auth file created by login action. Now replaced with podman rmi -a, which should not have such side effect.

Yeah, it cleared the auth file, just about to say that.

ntkme commented 3 years ago

@tetchel Done with refactoring and added documentation. PTAL. Thanks!

ntkme commented 3 years ago

Forgot the pre-commit hook. Let me update it.

tetchel commented 3 years ago

great change with the matrix to generate building w/ docker/podman/fqin

divyansh42 commented 3 years ago

We have released this feature in v2.4 However, you should be able to use this in push-to-registry@v2 also.