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

imagebuildah: Support custom image reference lookup for cache push/pull #5532

Conversation

aaronlehmann
Copy link
Contributor

What type of PR is this?

/kind api-change

What this PR does / why we need it:

This allows callers to provide custom SourceLookupReferenceFunc and DestinationLookupReferenceFunc for cache pull/push. These can be used to implement custom blob caches, and to wrap the reference being pushed/pulled to influence the copy behavior.

How to verify it

Verification isn't really necessary since this is just plumbing through functions for other code calling into buildah, but one could write a test program that embeds buildah and supplies its own funcs here.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

n/a

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 16, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan
Copy link
Member

rhatdan commented May 16, 2024

Please add tests.

@aaronlehmann
Copy link
Contributor Author

Please add tests.

I think this is a tricky thing to construct a test for since it requires pushing cache to/from a registry. The integration tests do this, but they use a real buildah binary that doesn't use these options.

I saw there is a TestConformance test which exercises imagebuildah.BuildDockerfiles, but this doesn't have a registry available to test with, and hacking either hacking that into this test or creating a similar test with a registry feels like it would involve creating a lot of infrastructure just to test something extremely simple.

One option would be to add hidden flags to the buildah build subcommand that rewrite the cache source/destination refs (basically just for testing purposes - I don't think they would have a practical use), and exercise that in integration tests. Does that sound like a good approach? Do you have any other ideas?

@rhatdan
Copy link
Member

rhatdan commented May 18, 2024

No after looking at the PR more closely, I think we can let it in without tests.

@rhatdan rhatdan added the No New Tests Allow PR to proceed without adding regression tests label May 18, 2024
@aaronlehmann
Copy link
Contributor Author

Cool, are there any other changes you'd like here?

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

I am just randomly passing by and I am expressing no opinion on the cost/benefit of adding these indirections.

pull.go Show resolved Hide resolved
define/build.go Outdated Show resolved Hide resolved
@aaronlehmann aaronlehmann force-pushed the source-dest-lookup-reference-funcs branch from 53d0571 to 7d61a74 Compare May 24, 2024 19:51
This allows callers to provide custom SourceLookupReferenceFunc and
DestinationLookupReferenceFunc for cache pull/push. These can be used to
implement custom blob caches, and to wrap the reference being
pushed/pulled to influence the copy behavior.

Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
@aaronlehmann aaronlehmann force-pushed the source-dest-lookup-reference-funcs branch from 7d61a74 to 7ff83d5 Compare May 24, 2024 21:27
Signed-off-by: Aaron Lehmann <alehmann@netflix.com>
@aaronlehmann aaronlehmann force-pushed the source-dest-lookup-reference-funcs branch from b25c968 to 9521672 Compare May 27, 2024 17:50
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

Implementation LGTM. I’ll let others decide about the cost/benefit of the feature.

@rhatdan
Copy link
Member

rhatdan commented May 28, 2024

/lgtm
/approve
Thanks @aaronlehmann

Copy link
Contributor

openshift-ci bot commented May 28, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaronlehmann, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 9f0f6d7 into containers:main May 28, 2024
32 checks passed
@aaronlehmann aaronlehmann deleted the source-dest-lookup-reference-funcs branch May 28, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm No New Tests Allow PR to proceed without adding regression tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants