Remove delegation to RNode in Resource #3888
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: monopole The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
monopole
requested review from
natasha41575
and removed request for
justinsb and
KnVerey
|
@KnVerey fyi, i think i mentioned this to you once as a possible simplification step. we can go further in this vein. |
k8s-ci-robot
added
size/L
|
|
||
| func (r *Resource) SetBinaryDataMap(m map[string]string) { | ||
| r.node.SetBinaryDataMap(m) | ||
| return h.Hash(&r.RNode) | ||
| } | ||
|
|
||
| func (r *Resource) SetGvk(gvk resid.Gvk) { |
There was a problem hiding this comment.
Should we push this one and GetGvk down to RNode?
There was a problem hiding this comment.
Want to, but at the moment, kyaml/resid (where gvk.go lives) depends on kyaml/yaml (which is where rnode.go lives). So we'd get a circular dependence if we made RNode depend on Gvk. Still whittling away at this...,
| @@ -107,6 +107,7 @@ func (ra *ResAccumulator) findVarValueFromResources(v types.Var) (interface{}, e | |||
| for _, res := range ra.resMap.Resources() { | |||
| for _, varName := range res.GetRefVarNames() { | |||
| if varName == v.Name { | |||
| //nolint: staticcheck | |||
There was a problem hiding this comment.
suppresses a lint failure, because the next line is calling a deprecated function.
We need to remove that call, but its outside the scope of this pr
| func (r *Resource) AllowKindChange() { | ||
| annotations := r.GetAnnotations() | ||
| annotations[buildAnnotationAllowKindChange] = allowed | ||
| r.SetAnnotations(annotations) | ||
| if err := r.SetAnnotations(annotations); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
why panic instead of returning the error?
There was a problem hiding this comment.
i'd have to change the signature of AllowKindChange and propagate up. I tried to only panic where that wasn't a possibilty already.
frankly i'm about ready to remove error from the SetLabels and SetAnnotations signature.
edit:
I'm pretty sure it only fails if the underlying object is nil (which means its unlikely execution will ever get there anyway)
They will even add a metadata field if it's missing (which is arguably too accommodating).
Even if we remove the error from SetLabels and SetAnnotations return, we'd have to panic inside those functions rather than swallow the error.
There was a problem hiding this comment.
cool, thanks for the explanation :)
There was a problem hiding this comment.
what do you folks think? remove the error return from SetLabels and SetAnnotations, and panic inside them if there's an error? There's no way to recover, because the node would have to be nil and that's a programmer error anyway.
There was a problem hiding this comment.
As long as there's no user error that could trigger this panic, I think it's fine as it is
| func (r *Resource) AllowNameChange() { | ||
| annotations := r.GetAnnotations() | ||
| annotations[buildAnnotationAllowNameChange] = allowed | ||
| r.SetAnnotations(annotations) | ||
| if err := r.SetAnnotations(annotations); err != nil { | ||
| panic(err) |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
natasha41575
removed
the
do-not-merge/hold
Close #3885
Builds on #3886 (that should go first) - it's the same as the first commit in this PR.
ALLOW_MODULE_SPAN
Also, this PR stops swallowing errors on
SetLabelandSetAnnotationscalls. It's rare those should fail, but if they do it's better to fail than swallow.