Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Start to flesh out crane optimize. #879
Start to flesh out crane optimize. #879
Changes from 2 commits
b6faed6
f4b6650
c1b9c5c
fbf72a6
4ca4734
48acc2c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Because this is hidden, I don't really care that much, but I would rather this be called something like
[e]stargz[ify]
(and documented as such), because "optimize" doesn't mean much.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'll let you and @imjasonh fight about this. That was my original proposal and I care almost not at all.
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.
It would be nice if this allowed one arg and would just retag the optimized version, with an optional
-t
flag for the dst if you want to leave src alone.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.
IDK how I feel about in-place optimization, but most of the places I want this for are things that aren't yet producing these images.
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.
My "as long as it's hidden" concession for the command also kind of applies here. I don't think we should expose this in pkg/crane until we're happy with exposing it in the CLI as well.
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.
Given how things are structured, that isn't particularly feasible (without deviating significantly from the broader style). What did you have in mind?
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.
You could keep this under cmd/crane/cmd, which is what I've done for
gcrane
stuff I don't want to expose.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.
What you're doing follows the patterns from
crane.Copy
, which makes sense, but it would be nice to expose this separately as well, maybeOptimizeImage
, so it can be used with non-remote images.It might also make sense for some of this boilerplate to live in
mutate
.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 may drop this as I'm not sure it is actually correct.
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.
We really need a:
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 believe I have one downstream somewhere 😇
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.
https://github.com/mattmoor/mink/blob/4afdeb3398c4d56401d4638ebd5cb2cb85ad12ad/pkg/bundles/map.go#L118
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.
Oh yeah you cced me on this earlier. My only issue is the pushing bit, we are missing an abstraction here for an ImageOrIndex thing (strawman here: #835)
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.
We could probably do something in
mutate
to make this a little nicer.