enhancement: drop the redundant `ApplyUnstructuredResourceImproved` return value #1833

rexagod · 2024-10-14T20:17:23Z

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 [email protected]

Blocked by (and rebased over) #1823. The relevant commit is 12f2bce.

rexagod · 2024-10-14T20:17:49Z

(cc @jan--f)

test/e2e-monitoring/monitoring_test.go

pkg/operator/resource/resourceapply/monitoring.go

jan--f · 2024-11-05T09:01:04Z

Thanks, I like the slimmer interface.

/lgtm

rexagod · 2024-11-06T10:50:20Z

Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial for approval.

/cc @p0lyn0mial

rexagod · 2024-11-11T16:53:11Z

cc @deads2k @p0lyn0mial to PTAL 🙏🏼

p0lyn0mial · 2024-11-14T08:59:57Z

pkg/operator/resource/resourceapply/monitoring.go

@@ -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)


Okay, what is the plan for switching to ApplyUnstructuredResourceImproved?

I was thinking of doing that early in the next release to allow enough time for folks to migrate?

OK, so the only difference between these two functions is the number of returned values. Am I right?

Exactly, nothing else.

rexagod · 2025-01-15T08:58:30Z

Friendly ping, @p0lyn0mial for another look here please. I think the deprecation cycle should continue for a couple of releases to allow enough soak time.

…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 <[email protected]>

openshift-ci · 2025-01-15T12:27:38Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-01-15T12:27:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jan--f, rexagod
Once this PR has been reviewed and has the lgtm label, please assign p0lyn0mial for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/operator/OWNERS
test/e2e-monitoring/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2025-01-15T13:40:22Z

@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.

p0lyn0mial · 2025-01-24T09:52:46Z

pkg/operator/resource/resourceapply/monitoring.go

@@ -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) {


So your plan is to change the methods, like ApplyPrometheus, to use ApplyUnstructuredResourceImproved, and at the same time, you would also break compatibility?

I think so, down the line after enough time has passed, since the bool value is redundant.

p0lyn0mial · 2025-01-24T09:58:57Z

pkg/operator/resource/resourceapply/monitoring.go

@@ -42,7 +42,7 @@ func DeletePrometheus(ctx context.Context, client dynamic.Interface, recorder ev

 // ApplyPrometheusRule applies the PrometheusRule.
 func ApplyPrometheusRule(ctx context.Context, client dynamic.Interface, recorder events.Recorder, required *unstructured.Unstructured) (*unstructured.Unstructured, bool, error) {
-	return ApplyUnstructuredResourceImproved(ctx, client, recorder, required, noCache, prometheusRuleGVR, nil, nil)


Also, what is the migration path? Do you know how the redundant parameter is used?

And do you know how many users of ApplyUnstructuredResourceImproved we have? Based on. https://github.com/search?q=org%3Aopenshift%20ApplyUnstructuredResourceImproved&type=code, not that many :)

Is there a value in introducing ApplyUnstructuredResourceImprovedDeprecated ?
Wouldn't be better to change the methods like ApplyAlertmanager ?

Also, what is the migration path? Do you know how the redundant parameter is used?

The parameter is used to check if the function actually updated the object IIRC. However, this is redundant since that could be done based on the returned object, and hence that parameter does not need to be baked into the existing machinery.

And do you know how many users of ApplyUnstructuredResourceImproved we have? Based on. github.com/search?q=org%3Aopenshift%20ApplyUnstructuredResourceImproved&type=code, not that many :)

I see. This is in-line with the fact that we ourselves use the wrapper ApplyX methods instead downstream. However, the ApplyUnstructuredResourceImproved change here breaks these functions too, as you mentioned above, namely in:

openshift/cluster-kube-apiserver-operator

openshift/multiarch-tuning-operator

openshift/loki

openshift/cluster-storage-operator

Is there a value in introducing ApplyUnstructuredResourceImprovedDeprecated ?
Wouldn't be better to change the methods like ApplyAlertmanager ?

In the future, we want to move the focus away from ApplyX functions, as that implies we'd need to keep library-go in sync with every arbitrary X monitoring CRD that is being used downstream.

This essentially creates a loop where an ApplyX function isn't available for X, so downstream uses ApplyUnstructuredResourceImproved for the time being, before switching to ApplyX when it gets merged here, which itself will always be nothing more than a caller for it, and it would rid us of the unnecessary maintenance that comes as a side-effect (users will effectively need to nil the last two fields to achieve the same effect, as above).

EDIT: Sorry, scratch that, I forgot that ApplyX is a part of a conventional pattern across the library, so it'd make sense to keep this so users can keep the same ApplyX expectations for monitoring types.

Nonetheless, since all of these are essentially calling wrappers, focusing on ApplyUnstructuredResourceImproved would make it all the more easier for us to maintain (especially helpful when a ApplyX won't exist, or if we need a more granular control) rather than duplicating the logic across ApplyXs (as the entirety of the update pattern is the same for all Xs).

However, this is redundant since that could be done based on the returned object, and hence that parameter does not need to be baked into the existing machinery.

I think that returning a parameter indicating whether the object was updated is slightly more convenient than checking the object itself. It is not obvious how to do it. In practice, I think it would involve creating a common function to be used by the callers.

Additionally, the signature of the Apply functions aligns with the signature exposed by the Apply functions for other resources. All return the additional parameter. Here are a few examples:

ApplyDeployments

ApplyNamespaces

ApplyClusterRole

Does it make sense ?

It does, and seeing that this has been the convention, it wouldn't be nice to break user expectations. Closing, thank you for your review here!

openshift-ci bot requested review from deads2k and jsafrane October 14, 2024 20:17

rexagod mentioned this pull request Oct 14, 2024

OCPBUGS-17113: Lazy updates for Prometheus openshift/cluster-monitoring-operator#2395

Open

2 tasks

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2024

rexagod force-pushed the regression-1575-followup branch from 12f2bce to 5a7e66e Compare October 20, 2024 10:57

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2024

rexagod commented Oct 20, 2024

View reviewed changes

test/e2e-monitoring/monitoring_test.go Show resolved Hide resolved

rexagod commented Nov 5, 2024

View reviewed changes

pkg/operator/resource/resourceapply/monitoring.go Show resolved Hide resolved

openshift-ci bot assigned jan--f Nov 5, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2024

openshift-ci bot requested a review from p0lyn0mial November 6, 2024 10:50

p0lyn0mial reviewed Nov 14, 2024

View reviewed changes

rexagod requested a review from p0lyn0mial November 18, 2024 09:59

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2025

enhancement: drop the redundant ApplyUnstructuredResourceImproved ret…

d27edb4

…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 <[email protected]>

rexagod force-pushed the regression-1575-followup branch from 5a7e66e to d27edb4 Compare January 15, 2025 12:27

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2025

p0lyn0mial reviewed Jan 24, 2025

View reviewed changes

rexagod closed this Jan 29, 2025

		@@ -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) {

enhancement: drop the redundant ApplyUnstructuredResourceImproved return value #1833

enhancement: drop the redundant ApplyUnstructuredResourceImproved return value #1833

Uh oh!

Conversation

rexagod commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rexagod commented Oct 14, 2024

Uh oh!

Uh oh!

Uh oh!

jan--f commented Nov 5, 2024

Uh oh!

rexagod commented Nov 6, 2024

Uh oh!

rexagod commented Nov 11, 2024

Uh oh!

p0lyn0mial Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

rexagod Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

rexagod Jan 27, 2025

Choose a reason for hiding this comment

Uh oh!

rexagod commented Jan 15, 2025

Uh oh!

openshift-ci bot commented Jan 15, 2025

Uh oh!

openshift-ci bot commented Jan 15, 2025

Uh oh!

openshift-ci bot commented Jan 15, 2025

Uh oh!

p0lyn0mial Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rexagod Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

rexagod Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

rexagod Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enhancement: drop the redundant `ApplyUnstructuredResourceImproved` return value #1833

enhancement: drop the redundant `ApplyUnstructuredResourceImproved` return value #1833

rexagod commented Oct 14, 2024 •

edited

Loading

p0lyn0mial Jan 24, 2025 •

edited

Loading

rexagod Jan 27, 2025 •

edited

Loading

rexagod Jan 27, 2025 •

edited

Loading