-
Notifications
You must be signed in to change notification settings - Fork 288
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
k8s: inject pull policies into crds #4304
Conversation
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!
internal/k8s/jsonpath/value.go
Outdated
func (v *Value) Sibling(name string) (val Value, ok bool) { | ||
key := reflect.ValueOf(name) | ||
sib := v.parentMap.MapIndex(key) | ||
if sib == (reflect.Value{}) { |
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 sib.IsZero()
would be more idiomatic
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 a great error message: panic: reflect: call of reflect.Value.IsZero on zero Value
It looks like:
Value.IsZero
returns true if it's got the value's zero value, and panics otherwise.MapIndex
returns an untyped zero valueValue.IsValid
is what we want for this case
internal/k8s/locator.go
Outdated
@@ -121,6 +125,10 @@ func (l *JSONPathImageLocator) Inject(e K8sEntity, selector container.RefSelecto | |||
if selector.Matches(ref) { | |||
val.Set(reflect.ValueOf(container.FamiliarString(injectRef))) | |||
modified = true | |||
|
|||
if pullPolicyVal, ok := val.Sibling(imagePullPolicyKey); ok { |
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 also check pullPolicyVal.CanSet()? always get nervous about Value.Set panicking
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.
ha, did you see the CI failure?
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.
lol, nope i did not! 🌟
Fixes #4291
Problem
For standard k8s objects, Tilt will inject an image pull policy. For CRDs, it will not.
It's relatively common for CRDs to use the normal k8s container spec, with "image" and "imagePullPolicy" fields. Tilt provides a way for users to configure how to inject the image ref, but leave users to their own devices when it comes to imagePullPolicy.
Solution
If a CRD "image" field has an "imagePullPolicy" neighbor, inject the pull policy.
Note: the image object locator (where you specify, e.g., a repo and ref field) just ignores the pull policy. An obvious answer here is to add an option to specify the pull policy field. My weak gut instinct is it's not worth the effort to worry about that until someone complains. I'm happy to do that as well if someone has a different gut instinct.