-
Notifications
You must be signed in to change notification settings - Fork 420
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
Add support for podman image digests #5154
base: mainline
Are you sure you want to change the base?
Conversation
🍕 Here are the new binary sizes!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great and thank you for contributing! Just wanted to double check: have you tested this feature locally and verified you would be able to build images using podman after this change? Or is there anything else that needs to be done for the full workflow?
@@ -170,12 +171,22 @@ func (c DockerCmdClient) Login(uri, username, password string) error { | |||
} | |||
|
|||
// Push pushes the images with the specified tags and ecr repository URI, and returns the image digest on success. | |||
func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags ...string) (digest string, err error) { | |||
func (c DockerCmdClient) Push(ctx context.Context, uri string, engine string, w io.Writer, tags ...string) (digest string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like we should have a different function for podman push (same signature as Push
it's just with a different name), so that we don't need to change the function signature and also the push function is quite different when the engine type is podman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider this, but it seems like there's still a lot of shared functionality that would be a pain to update in multiple places.
The alternative could be to build a new type PodmanCmdClient that implements ContainerLoginBuildPusher by just calling the existing methods - but again that seemed to add more complexity than it removed.
@mjrlee, are there any updates on this, or do you need support? 😁 |
Sorry, I haven't had much time to look at this recently. I can confirm I'm currently using it with podman and it's working well. I've resolved a couple of comments on the PR, and replied to another. |
Adds a new "engine" build argument that can be used to handle podman's different way of handing image digests.
Fixes #3170
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.