-
Notifications
You must be signed in to change notification settings - Fork 528
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
added support for overriding manifest annotations #1056
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1056 +/- ##
==========================================
- Coverage 75.46% 75.34% -0.12%
==========================================
Files 107 107
Lines 5087 5095 +8
==========================================
Hits 3839 3839
- Misses 703 711 +8
Partials 545 545
Continue to review full report at Codecov.
|
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 for this change! 🎉
pkg/v1/mutate/mutate.go
Outdated
m, err := base.Manifest() | ||
if err != nil { | ||
return nil, err | ||
} |
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.
Can we m.DeepCopy
here instead, as early as possible and before any changes?
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.
thank you so much for this review, but I tried to do that but it didn't work for me, I updated the code with the following content, what did I do wrong?
mn := m.DeepCopy()
if mn.Annotations == nil {
mn.Annotations = map[string]string{}
}
for k, v := range annotations {
mn.Annotations[k] = v
}
image := &image{
base: base,
manifest: mn,
}
pkg/v1/mutate/mutate_test.go
Outdated
t.Fatalf("failed to mutate a config: %v", err) | ||
} | ||
|
||
if manifestsAreEqual(t, source, result) { |
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.
Does this indicate that the source was mutated in the process? I would expect source to be unmodified and the modified result to be returned.
I'll add some context from #281 we talked about how this would work for both image and manifest lists. I don't think you need to solve this immediately, but being able to annotate just the top-level manifest vs recursing and annotating every manifest is an interesting problem. |
pkg/v1/mutate/mutate.go
Outdated
@@ -113,6 +113,29 @@ func Config(base v1.Image, cfg v1.Config) (v1.Image, error) { | |||
return ConfigFile(base, cf) | |||
} | |||
|
|||
// IndexAnnotations mutates the provided v1.Image to have the provided annotations | |||
func IndexAnnotations(base v1.Image, annotations map[string]string) (v1.Image, 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.
Sorry, my comment wasn't clear, just call this Annotations
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.
done.
Oh yeah, you'll want to do this lazily. That
One thing I'm a little worried about is that if you run this command on a manifest list, I think it will turn that manifest list into an image, because we just call Give me one second, I want to implement something that will let us check if something is an image. |
|
thank you so much for this, I really appreciate it, I updated the PR according to your feedback. 🙋🏻♂️😋 |
where should we do this validation, would you like to help me if you don't mind? 🙏 @jonjohnsonjr |
So at the beginning of the command, before we if len(anntns) != 0 {
desc, err := crane.Head(ref, *options...)
if err != nil {
log.Fatalf("checking %s: %v", ref, err)
}
if desc.MediaType.IsIndex() {
log.Fatalf("mutating annotations on an index is not yet supported")
}
} |
done @jonjohnsonjr in the commit de9902f but I don't understand the necessity of doing this, can you please tell me why we are doing this, and what is the index thing actually, thank you 🙏 |
Sure, let's look at This is a multi-platform image, AKA an image index, which I often shorten to just "index". Per the spec, an image index can have annotations, so it would be reasonable to expect calling However, this will not work! It would instead add the annotation to one of it's children -- the linux/amd64 image: Why? Because After annotating the child image, it would update the We want to avoid that last part, so if we encounter something that isn't an image, we'll just abort. In the future, we could add something like |
Looks like the linter is mad, need to run |
d5e8970
to
50d14fe
Compare
Omg, thank you so much for this detailed explanation, I understand now 👍 |
fixed, thank you so much 👍 |
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com> Co-Authored-by: Jon Johnson <jonjohnson@google.com> - improvements based on feedbacks - check to make sure we're not trying to mutate annotations on an index
Thanks! |
Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com
Hello, this PR will add overriding manifest annotations support to crane's
mutate
command, it will work like the following picture: