fix: filter labels in direct ComputeForwardingRule controller #4808
Conversation
27d4808 to
f25f69b
Compare
|
/lgtm I'm okay with merging this to unblock release 1.133. Further questions:
cc: @yuwenma |
| labels: | ||
| label-one: "value-one" | ||
| foo/not-valid-in-gcp: bar | ||
| foo: bar |
There was a problem hiding this comment.
nit: foo: bar seems redundant as we have label-one: "value-one". To test update, we could simply update it to label-one: "value-two" in below update.yaml.
There was a problem hiding this comment.
Can we remove line 110? (managed-by-cnrm)
| mapCtx := &direct.MapContext{} | ||
|
|
||
| desired := a.desired.DeepCopy() | ||
| sanitizedLabels := label.NewGCPLabelsFromK8sLabels(desired.Labels) |
There was a problem hiding this comment.
This meta label will override the spec labels, but I think we want the opposite:
- If spec.labels are purposely used, we ignore meta labels.
Can we uselabels.ComputeLabelsin adapter initialization so we can get rid of these code changes?
There was a problem hiding this comment.
We don't support spec.labels in this PR yet, so I'm not concerned about the override and it's fine to keep the code as is. I'll use labels.ComputeLabels once we add the spec.labels field.
There was a problem hiding this comment.
I see. That makes sense. So basically this PR handles the direct controller backward compatibility support for labels, and in the future we can add the spec.labels.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma 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 |
b6602eb
into
GoogleCloudPlatform:master
No description provided.