enhancement: drop the redundant ApplyUnstructuredResourceImproved return value
#1833
Conversation
|
(cc @jan--f) |
openshift-merge-robot
added
the
needs-rebase
…urn value Drop the boolean return value that signified if an update happened as it is not necessary and pollutes the signature. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>
rexagod
force-pushed
the
regression-1575-followup
branch
from
12f2bce to
5a7e66e
Compare
openshift-merge-robot
removed
the
needs-rebase
| unstructuredResource.SetResourceVersion(gotUnstructuredResource.GetResourceVersion()) | ||
| unstructuredResource.SetGeneration(gotUnstructuredResource.GetGeneration()) | ||
| unstructuredResource.SetCreationTimestamp(gotUnstructuredResource.GetCreationTimestamp()) | ||
| unstructuredResource.SetUID(gotUnstructuredResource.GetUID()) | ||
| unstructuredResource.SetManagedFields(gotUnstructuredResource.GetManagedFields()) | ||
| require.Equal(t, unstructuredResource.UnstructuredContent(), gotUnstructuredResource.UnstructuredContent()) |
There was a problem hiding this comment.
Since we are already doing this now, LMK if the didUpdate function above feels redundant now. I kept it to visualize the workaround for the didUpdate return parameter in the test.
|
@rexagod: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| @@ -72,6 +72,26 @@ func ApplyUnstructuredResourceImproved( | |||
| resourceGVR schema.GroupVersionResource, | |||
| defaultingFunc mimicDefaultingFunc, | |||
| equalityChecker equalityChecker, | |||
| ) (*unstructured.Unstructured, error) { | |||
There was a problem hiding this comment.
@jan--f I believe this satisfies the proposed refactor we talked about earlier?
|
Thanks, I like the slimmer interface. /lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jan--f, rexagod The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/cc @p0lyn0mial |
|
cc @deads2k @p0lyn0mial to PTAL 🙏🏼 |
| @@ -32,7 +32,7 @@ func DeleteAlertmanager(ctx context.Context, client dynamic.Interface, recorder | |||
|
|
|||
| // ApplyPrometheus applies the Prometheus. | |||
| func ApplyPrometheus(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) { | |||
| return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, prometheusGVR, nil, nil) | |||
| return ApplyUnstructuredResourceImprovedDeprecated(ctx, client, recorder, required, noCache, prometheusGVR, nil, nil) | |||
There was a problem hiding this comment.
Okay, what is the plan for switching to ApplyUnstructuredResourceImproved?
There was a problem hiding this comment.
I was thinking of doing that early in the next release to allow enough time for folks to migrate?
Drop the boolean return value that signified if an update happened as it
is not necessary and pollutes the signature.
Signed-off-by: Pranshu Srivastava rexagod@gmail.com
Blocked by (and rebased over) #1823. The relevant commit is 12f2bce.