Make patching of reference resource optional #169
Conversation
…-replicas annotation to optionally disable patching of reference resource when using scaling based on reference resource.
pr00se
left a comment
pr00se
left a comment
There was a problem hiding this comment.
Also needs a CHANGELOG entry, but otherwise LGTM. Thanks!
README.md
Outdated
| * `grafana.com/rollout-mirror-replicas-from-resource-name` | ||
| * `grafana.com/rollout-mirror-replicas-from-resource-kind` | ||
| * `grafana.com/rollout-mirror-replicas-from-resource-api-version` | ||
| * `grafana.com/rollout-mirror-replicas-from-resource-update-status-replicas` |
There was a problem hiding this comment.
nit: what do you think of something like -write-back-status-replicas? might a bit clearer about which status.replicas is updated (the reference resource's).
There was a problem hiding this comment.
Sounds good to me.
| logger = log.With(logger, "name", sts.GetName(), "replicas", replicas, "referenceResource", referenceResource) | ||
|
|
||
| // If annotation is not present, or equals to "true", we update. If annotation equals to "false" or fails to parse, we don't update. | ||
| updateReplicas, ok := sts.Annotations[config.RolloutMirrorReplicasFromResourceUpdateStatusReplicas] |
There was a problem hiding this comment.
This is a question that has nothing to do with the actual code 😄
Sometimes we use sts.GetAnnotations() and sometimes sts.Annotations to fetch the annotations -- is there any actual difference, or just two ways to do the same thing (the function just returns sts.Annotations so I'm guessing they're the same)?
There was a problem hiding this comment.
Tbh, I just found some random place where we did the same and used the same approach. As you found, it doesn't make a difference here.
pkg/controller/controller_test.go
Outdated
| expectedPatchedSets: nil, | ||
| expectedPatchedResources: map[string][]string{"my.group/v1/customresources/test/status": {`{"status":{"replicas":3}}`}}, | ||
| }, | ||
| "should patch scale subresource status.replicas if it doesn't match statefulset, but not patch the resource since it's disabled": { |
There was a problem hiding this comment.
nit: "should patch...but not patch" is confusing -- maybe "shouldn't patch resource even if status.replicas doesn't match statefulset since patching is disabled"
|
Thanks for review @pr00se, I hopefully addressed all your findings. |
pstibrany
deleted the
optionally-disable-patching-of-reference-resource
branch
2 participants
Added grafana.com/rollout-mirror-replicas-from-resource-update-status-replicas annotation to optionally disable patching of reference resource when using scaling based on reference resource.