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

Add support for podman image digests #5154

Open
wants to merge 3 commits into
base: mainline
Choose a base branch
from
Open

Conversation

mjrlee
Copy link

@mjrlee mjrlee commented Aug 3, 2023

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.

@mjrlee mjrlee requested a review from a team as a code owner August 3, 2023 10:20
@mjrlee mjrlee requested review from efekarakus and removed request for a team August 3, 2023 10:20
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 51492 51488 +0.01
macOS (arm) 51680 51684 ❤️ -0.01
linux (amd) 45324 45324 ❤️ 0.00
linux (arm) 43584 43588 ❤️ -0.01
windows (amd) 42144 42144 ❤️ 0.00

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a 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?

internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved
@@ -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) {
Copy link
Contributor

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.

Copy link
Author

@mjrlee mjrlee Feb 13, 2024

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.

@bvtujo bvtujo added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Aug 31, 2023
@Lou1415926 Lou1415926 added area/addon Issues about addons. do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed area/addon Issues about addons. do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Oct 4, 2023
@Lou1415926 Lou1415926 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Nov 9, 2023
@iamhopaul123 iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Dec 13, 2023
@iamhopaul123 iamhopaul123 added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 17, 2024
@ammerzon
Copy link

@mjrlee, are there any updates on this, or do you need support? 😁

@KollaAdithya KollaAdithya added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jan 30, 2024
@mjrlee
Copy link
Author

mjrlee commented Feb 13, 2024

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.

@KollaAdithya KollaAdithya added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Mar 21, 2024
@huanjani huanjani added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 18, 2024
@huanjani huanjani removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
In review
Development

Successfully merging this pull request may close these issues.

Support Podman either as alias to Docker or natively
7 participants