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

Refactor inputs to support docker/metadata-action #50

Merged
merged 2 commits into from
Oct 12, 2021
Merged

Refactor inputs to support docker/metadata-action #50

merged 2 commits into from
Oct 12, 2021

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Oct 6, 2021

This PR adds support for docker/metadata-action@v2. See redhat-actions/buildah-build#74 and redhat-actions/buildah-build#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)

redhat-actions/buildah-build#74
redhat-actions/buildah-build#76

Checklist

  • This PR includes a documentation change
  • This PR does not need a documentation change

  • This PR includes test changes
  • This PR's changes are already tested

  • This change is not user-facing
  • This change is a patch change
  • This change is a minor change
  • This change is a major (breaking) change

Changes made

Support docker/metadata-action

Copy link
Member

@divyansh42 divyansh42 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution 👍
This PR looks good to me.
Could please add test workflows, so that we can verify the changes.
Also it will be good if you can modify the inputs description in action.yml and in README as this PR change the uses of those.

@tetchel
Copy link
Contributor

tetchel commented Oct 7, 2021

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
Copy link
Contributor

tetchel commented Oct 7, 2021

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

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

@divyansh42
Copy link
Member

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

@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
Copy link
Member

divyansh42 commented Oct 8, 2021

@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.

Copy link
Member

@divyansh42 divyansh42 left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.
I have suggested few minor changes (that was present earlier also, but I noticed that in this PR) that you may want to address. Otherwise, I think functionality-wise we are good.

But I'll let @tetchel to review the changes and approve the PR.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@divyansh42
Copy link
Member

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

@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 ntkme requested a review from divyansh42 October 8, 2021 17:37
@ntkme
Copy link
Contributor Author

ntkme commented Oct 8, 2021

@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
Copy link
Member

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
Copy link
Contributor Author

ntkme commented Oct 8, 2021

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

README.md Show resolved Hide resolved
@ntkme
Copy link
Contributor Author

ntkme commented Oct 12, 2021

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

@tetchel
Copy link
Contributor

tetchel commented Oct 12, 2021

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

Copy link
Member

@divyansh42 divyansh42 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing those reviews.
Looks good now 👍

@tetchel tetchel changed the title Support docker/metadata-action Refactor inputs to support docker/metadata-action Oct 12, 2021
@tetchel tetchel merged commit bdfa69a into redhat-actions:main Oct 12, 2021
@divyansh42
Copy link
Member

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

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