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

RFC: Consolidate getting signatures #1417

Merged
merged 2 commits into from Jul 21, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Nov 29, 2021

Consolidate figuring out which signatures from the source to copy, and whether that can work, into a helper.

@vrothberg RFC — I’m honestly rather unhappy about passing the progress messages into the helper (and, at the same time, unwilling to make the messages less descriptive just to share a few lines of code); basically just to show that something like this is possible, and solicit better ideas.

At this point I’d really prefer only the first commit to be merged.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Both look like an improvement to me.

LGTM

@mtrmac mtrmac force-pushed the consolidate-GetSignatures branch 6 times, most recently from eded5a3 to 2d44e7b Compare December 3, 2021 15:21
@mtrmac mtrmac force-pushed the consolidate-GetSignatures branch from 371b442 to adf86e3 Compare June 17, 2022 20:12
@mtrmac mtrmac force-pushed the consolidate-GetSignatures branch from adf86e3 to 312fb65 Compare July 1, 2022 16:25
@rhatdan rhatdan changed the title RFC: Consolidate geting signatures RFC: Consolidate getting signatures Jul 5, 2022
@mtrmac mtrmac force-pushed the consolidate-GetSignatures branch from 312fb65 to fe13b0e Compare July 20, 2022 19:32
@rhatdan
Copy link
Member

rhatdan commented Jul 21, 2022

LGTM
Is this still a draft?

This should not make a difference right now, because nothing else
uses the UnparsedImage, but it allows caching _in principle_
and it makes the code more similar to the copyOneImage version,
which we'll benefit from next.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac force-pushed the consolidate-GetSignatures branch from fe13b0e to 1bdda15 Compare July 21, 2022 13:44
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 21, 2022

I’m not that happy about the end state, but I don’t think I will think of a better approach with any more time, so let’s do that.

@mtrmac mtrmac marked this pull request as ready for review July 21, 2022 13:45
@mtrmac mtrmac merged commit 5a5cf3b into containers:main Jul 21, 2022
@mtrmac mtrmac deleted the consolidate-GetSignatures branch July 21, 2022 14:48
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.

None yet

3 participants