-
Notifications
You must be signed in to change notification settings - Fork 164
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 in the initial configuration to add the image labels #1448
Add in the initial configuration to add the image labels #1448
Conversation
Mostly opening for feedback and to learn what I'm missing. Open Q:
|
@@ -151,6 +152,8 @@ func (bb *builderBlder) WriteableImage() (v1.Image, error) { | |||
return nil, err | |||
} | |||
|
|||
image, err = imagehelpers.SetStringLabels(image, bb.additionalLabels) |
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.
Add an error check here.
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.
updated!
pkg/cnb/create_builder.go
Outdated
@@ -65,6 +65,8 @@ func (r *RemoteBuilderCreator) CreateBuilder(ctx context.Context, builderKeychai | |||
builderBldr.AddGroup(buildpacks...) | |||
} | |||
|
|||
builderBldr.additionalLabels = spec.AdditionalLabels |
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.
All the other steps are modifications via setters on the builderBldr. Can we add a set additional labels?
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.
happy to, do you think it adds value though?
I guess said differently I'm probably going to have that setter effectively is going to say exactly what I say here - which may be because I'm not writing good go code or something
Thank you for the review btw @matthewmcnew |
Anything else y'all needed from me here? |
@@ -36,7 +36,8 @@ type BuilderSpec struct { | |||
Stack corev1.ObjectReference `json:"stack,omitempty"` | |||
Store corev1.ObjectReference `json:"store,omitempty"` | |||
// +listType | |||
Order []BuilderOrderEntry `json:"order,omitempty"` | |||
Order []BuilderOrderEntry `json:"order,omitempty"` | |||
AdditionalLabels map[string]string `json:"additionalLabels,omitempty"` |
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 you regenerate the CRDs 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.
I just added a new commit.
Please let me know if you think it's not the right place
f610a31
to
80f2e13
Compare
Signed-off-by: Alex Barbato <abarbato@vmware.com>
Add in the initial configuration to add the image labels
Closes #1442