-
Notifications
You must be signed in to change notification settings - Fork 527
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 to layout replacedescriptor and removedescriptor #848
Conversation
I like
In the same way we have If we get cute with type-casting, we could just have For your replace implementation, I would really expect:
but these are currently just equivalent to If just Remove then Append is your desired behavior, how would you feel about adding Remove to mutate, then doing the remove && append in your client code? I'd be okay with a "replace" for mutate if it did in-place and not just remove + append. |
Is it really? Given that
how would you place that in any other package?
Please not, or at least not yet. The need for distinct Image and ImageIndex in various places is an issue throughout, as you raised. I would rather be consistent here with what we do elsewhere, and if we find a way to unify it, then do it everywhere at once.
I really don't understand (which leaves me concerned others might not as well). I fully get that I can open a The above is complex to understand, especially if I already have a
I have no objection to adding What do I, as a newbie user of this library, call to remove or replace a manifest from a With this PR - or a variant - I can do: p, err := layout.FromPath(dir)
// OR
p, err := layout.Write(dir, empty.Index)
// and then
p.ReplaceDescriptor(desc, someMatcher) but now you are saying p, err := layout.FromPath(dir)
// OR
p, err := layout.Write(dir, empty.Index)
// and then
err = p.WriteImage(img)
index, err := p.ImageIndex()
// generate the descriptor from img, redoing everything that is at https://github.com/google/go-containerregistry/blob/3c3abfb82ca884fb7cc6db0efe77264f1c817f1f/pkg/v1/layout/write.go#L42-L69
desc := generateDescriptorFromImage(img)
index, err = mutate.ReplaceDescriptor(desc, someMatcher)
err = p.WriteIndex(index) // this is a function that does not exist now That is a lot messier and a lot more work than the original If you say, you want to keep the conveniences in |
3c3abfb
to
92435ad
Compare
I think I figured out what you meant @jonjohnsonjr . Take a look at it now. All of the mutation is done in |
Codecov Report
@@ Coverage Diff @@
## master #848 +/- ##
==========================================
- Coverage 74.33% 74.24% -0.09%
==========================================
Files 107 107
Lines 4523 4598 +75
==========================================
+ Hits 3362 3414 +52
- Misses 661 675 +14
- Partials 500 509 +9
Continue to review full report at Codecov.
|
OK, we have #841 in, which makes using this easier. Any other comments on this? |
pkg/v1/layout/write.go
Outdated
return err | ||
} | ||
|
||
mt, err := img.MediaType() |
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.
Use partial.Descriptor
here, for some bad reasons and some good reasons.
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 don't understand
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.
Sure, sorry I should have provided more details. It's two things:
- This is ~9 lines of code to produce a
v1.Descriptor
for an artifact, and we've consolidated that into apartial.Descriptor
method, just for brevity. - There are situations where you have some artifact, say a
v1.Image
, which was originally referenced by av1.ImageIndex
descriptor that contained platform information. If you just reconstruct thev1.Descriptor
as you're doing here, we lose that information. Thus, artifacts can optionally implementDescriptor() (v1.Descriptor, error)
to return the originalv1.Descriptor
by which they were referenced. This is especially interesting for foreign layers. See here and Add partial.Descriptor #654.
It unfortunately needs to live in partial
because the way we wrap things in partial means we shadow the wrapped artifact, but fortunately it makes sense to live there anyway.
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, I see that I bungled the multiline comment thing in GitHub. This comment should apply to lines 139-158.
I agree this is a problem, which maybe we can solve with docs and examples, but the "model" for how this library works will be hard to change without a big, breaking overhaul.
I think we're roughly on the same page, but if there's anything from this comment that I still need to address, let me know. |
I didn't understand the three comments above. Can you clarify? |
5b59c86
to
88adcf7
Compare
OK, now I think I got what you meant. See the latest. |
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.
Nice, I think this is a lot cleaner -- with tests this would lgtm.
88adcf7
to
da6be75
Compare
OK, lots of tests. Back at you @jonjohnsonjr |
8d0a120
to
90c2d31
Compare
3e271bc
to
bc13ec6
Compare
And rebased |
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.
Small suggestion to dedupe some code now that ReplaceDescriptor is removed, otherwise lgtm, feel free to push back if I'm missing something with my suggestion.
Oh, yeah, I like that. Nice and clean. |
bc13ec6
to
9f927af
Compare
Updated with your suggestions @jonjohnsonjr ; back at you. |
pkg/v1/mutate/mutate.go
Outdated
@@ -92,6 +93,26 @@ func AppendManifests(base v1.ImageIndex, adds ...IndexAddendum) v1.ImageIndex { | |||
} | |||
} | |||
|
|||
// RemoveManifests removes any descriptors that match the match.Matcher. | |||
func RemoveManifests(base v1.ImageIndex, matcher match.Matcher) (v1.ImageIndex, 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.
Okay, fine, one last thing before I merge this. Sorry... thank you for your patience.
We could defer this to be a little bit lazier (happens in compute
instead of here) so that we don't return an error here, which would make using it a little easier (one less error check, matching AppendManifests
).
This would allow for:
ii = mutate.AppendManifests(mutate.RemoveManifests(ii, matcher), add)
Not a huge deal, but something I often find frustrating when using this library is that I am constantly checking for errors for things that don't really need to return an error, and I wish I could go back and fix that.
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 understand what you are trying to do. If this only returns a v1.ImageIndex
and not (v1.ImageIndex, error)
, then you have the option to chain them.
I just don't see how.
The source for this is:
// RemoveManifests removes any descriptors that match the match.Matcher.
func RemoveManifests(base v1.ImageIndex, matcher match.Matcher) (v1.ImageIndex, error) {
ii, err := base.IndexManifest()
if err != nil {
return base, err
}
// create a list without any existing ones
manifests := []v1.Descriptor{}
for _, manifest := range ii.Manifests {
if !matcher(manifest) {
manifests = append(manifests, manifest)
}
}
return &index{
base: base,
replaces: manifests,
}, nil
}
The only potential error is from getting base.IndexManifest()
, but it could return an error. How would we handle it?
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.
but it could return an error. How would we handle it?
Drop index.replaces
, for an index.removeMatcher
and set it to the match.Matcher
we pass in, then instead of doing:
manifests := manifest.Manifests
if len(i.replaces) > 0 {
manifests = i.replaces
}
in index.compute
, do:
manifests := []v1.Descriptor
for _, desc := range manifest.Manifests {
if !i.removeMatcher(desc) {
manifests = append(manifests, desc)
}
}
We're basically just deferring that IndexManifest()
call until compute()
is called.
RemoveManifests
just becomes:
// RemoveManifests removes any descriptors that match the match.Matcher.
func RemoveManifests(base v1.ImageIndex, matcher match.Matcher) v1.ImageIndex {
return &index{
base: base,
removeMatcher: matcher,
}
}
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 think I got what you had in mind. I added it as another commit, so I can roll back if that is not it. Take a look and comment here. If it is, I will squash the two commits.
7bb7e15
to
f336189
Compare
pkg/v1/mutate/index.go
Outdated
@@ -90,6 +95,20 @@ func (i *index) compute() error { | |||
} | |||
manifest := m.DeepCopy() | |||
manifests := manifest.Manifests | |||
if len(i.replaces) > 0 { |
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 just unused now, right? We can drop index.replaces
.
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 can, but having figured it out how to work it, maybe we should just leave it?
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.
Having an unreachable/untested path is a bit strange to me, I'd just drop it.
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.
OK done.
pkg/v1/mutate/index.go
Outdated
// replaces are applied before adds | ||
replaces []v1.Descriptor | ||
// removes are removed after replaces and before adds | ||
removes []match.Matcher |
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 don't think this needs to be a list (yet?). In theory, we may be able to optimize some things in the future to do introspection and directly modify these fields instead of returning an entirely new mutate.index
each time, but right now this can only be a single matcher, I think.
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 probably can, but it parallels adds, for a clean API, and I made it similarly variadic. Let's just leave it.
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.
Eh... I'm not sure.
I do like that this mirrors AddManifests, but I think making this variadic is going to be a source of confusion. There's more than one way to compose matchers, and this interface forces the operator to only ever be "or".
You can achieve the same thing now with multiple calls to RemoveManifests
, which is definitely less efficient, but you can also just implement a match.Matcher
that matches multiple predicates.
Having RemoveManifests
accept multiple matchers encodes some matcher composition within mutate, and I'd rather that live in match
if we need it.
Of course, you know that I will advocate for adding match.Or
and match.And
to solve this, but we're not there yet :P
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.
OK done. And rebased.
f336189
to
a76ad68
Compare
Signed-off-by: Avi Deitcher <avi@deitcher.net>
a76ad68
to
3af4f8f
Compare
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.
Thanks :)
Fixes #842
adds the following to
v1/layout
:RemoveDescriptors(matcher match.Matcher)
- removes fromindex.json
any descriptors that matchReplaceDescriptor(desc v1.Descriptor, matcher match.Matcher)
- equivalent ofRemoveDescriptors(matcher)
followed byAppendDescriptor(desc)
ReplaceImage(image v1.Image, matcher match.Matcher)
- equivalent ofRemoveDescriptors(matcher)
followed byAppendImage(image)
ReplaceIndex(index v1.ImageIndex, matcher match.Matcher)
- equivalent ofRemoveDescriptors(matcher)
followed byAppendIndex(index)
None of these does garbage collection in the blobs dir, which is a completely separate topic, and should be handled under separate cover.