Fix status update #96

nebojsa-prodana · 2021-06-08T11:54:21Z

Fixes #93

controllers/progressivesync_controller.go

maruina

Some questions but looks promising.

api/v1alpha1/condition_types.go

controllers/progressivesync_controller.go

maruina · 2021-06-08T12:29:43Z

controllers/progressivesync_controller.go

+			return err
+		}
+		latest.Status = pr.Status
+		if err := r.Client.Status().Update(ctx, &latest); err != nil {


Why are we dropping the r.Client.Status().Patch(ctx, pr, client.MergeFrom(&latest)) and use Status().Update instead?

We don't need to merge anymore, because we already have the latest state retrieved, so we can just update instead.

internal/scheduler/scheduler.go

controllers/progressivesync_controller.go

internal/scheduler/scheduler.go

lgtm-com · 2021-06-08T14:13:14Z

This pull request introduces 1 alert when merging 1659d04 into 7f4bbae - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

nebojsa-prodana · 2021-06-08T14:22:14Z

Useless assignment to local variable

Seems like a false alert to me, the pr object is updated through pass by reference in the caller's code.

maruina · 2021-06-08T15:29:53Z

controllers/progressivesync_controller.go

-// patchStatus patches the progressive sync object status
-func (r *ProgressiveSyncReconciler) patchStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error {
+// submitStatus updates the progressive sync object status with backoff
+func (r *ProgressiveSyncReconciler) submitStatus(ctx context.Context, pr *syncv1alpha1.ProgressiveSync) error {


Why not updateStatus?

controllers/progressivesync_controller.go

maruina · 2021-06-08T15:56:17Z

controllers/progressivesync_controller.go

+			r.updateStageStatus(ctx, stage.Name, message, syncv1alpha1.PhaseUnknown, &pr)
+
+			if err := r.submitStatus(ctx, &pr); err != nil {
+				r.Log.Error(err, "failed to update object status")


Suggested change

r.Log.Error(err, "failed to update object status")

log.Error(err, "failed to update object status")

sledigabel · 2021-06-08T16:04:17Z

Quick summary of our conversation:

refactor the min calculation
check out all the r.Log and see if they can just be log.
refactor the initializeStatus to be closer to the format of the one in the helm controller: https://github.com/fluxcd/helm-controller/blob/e25d6894bfdc547101e60dbaf739f99ab4f005ef/controllers/helmrelease_controller.go#L184
same for submitStatus

3 and 4 should fix the problem we're seeing at the linting phase.
@maruina could pick this up in @nebojsa-prodana 's absence

DiogoMCampos · 2021-06-11T07:31:38Z

controllers/progressivesync_controller.go

+}
+
+// reconcileStage reconcile a ProgressiveSyncStage
+func (r *ProgressiveSyncReconciler) reconcileStage(ctx context.Context, ps syncv1alpha1.ProgressiveSync, stage syncv1alpha1.ProgressiveSyncStage) (syncv1alpha1.ProgressiveSync, reconcile.Result, error, bool) {


I'm not a fan of returning four different values, to be honest. I'm aware that it's idiomatic Go to return two values, with one being for error-handling, but having two additional return values makes this difficult to follow.

I think as a starting point, perhaps named return values (if at all possible?) and definitely adding documentation would make this better, as it's not immediately clear what the bool value means.

Further refactoring to avoid 3 non-error return values would probably be beneficial, but I understand if we'd rather do that later. 🙂

Well, I just saw https://github.com/fluxcd/helm-controller/blob/e25d6894bfdc547101e60dbaf739f99ab4f005ef/controllers/helmrelease_controller.go#L184 that Seb linked and can see they have 3 returns values (2 + error) - so this isn't a 'first' 🙂

I'm still not overly fond of multiple return values, but at least naming and/or documenting them something would be a good first step.

Truth is, the whole reconciliation loop has multiple responsibilities - or at least a wide scope of responsibility -, making it harder to have a single return value.

I was able to remove a variable because we can use result.Requeue to see if we need to requeue or not.

I don't think we can remove the ProgressiveSync variable unless we start doing things with pointer, but we decided to follow a value semantic.

Are you happy with the latest implementation? Is there anything else do you think we can improve?

Yea, looks good 👍

DiogoMCampos · 2021-06-11T08:00:18Z

controllers/progressivesync_controller.go

@@ -394,19 +293,14 @@ func (r *ProgressiveSyncReconciler) removeAnnotationFromApps(ctx context.Context
 }

 // updateStageStatus updates the target stage given a stage name, message and phase
-func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, message string, phase syncv1alpha1.StageStatusPhase, pr *syncv1alpha1.ProgressiveSync) error {
+func (r *ProgressiveSyncReconciler) updateStageStatus(ctx context.Context, name, message string, phase syncv1alpha1.StageStatusPhase, pr *syncv1alpha1.ProgressiveSync) {


Why have we removed the error return value here?

I think it's because pr.SetStageStatus doesn't return anything and we now make the update while calling updateStatusWithRetry.

api/v1alpha1/condition_types.go

controllers/progressivesync_controller.go

sledigabel

LGTM
well done all, great opportunity to fix a bug and refactor.

controllers/progressivesync_controller.go

nebojsa-prodana commented Jun 8, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

nebojsa-prodana added 2 commits June 8, 2021 14:14

Fix status update

1965fde

Clean the code a bit

ca87e60

nebojsa-prodana force-pushed the fix-status-update branch from dd68e91 to ca87e60 Compare June 8, 2021 12:15

maruina reviewed Jun 8, 2021

View reviewed changes

sledigabel reviewed Jun 8, 2021

View reviewed changes

controllers/progressivesync_controller.go Outdated Show resolved Hide resolved

sledigabel reviewed Jun 8, 2021

View reviewed changes

controllers/progressivesync_controller.go Outdated Show resolved Hide resolved

sledigabel reviewed Jun 8, 2021

View reviewed changes

internal/scheduler/scheduler.go Outdated Show resolved Hide resolved

nebojsa-prodana added 2 commits June 8, 2021 15:43

PR remarks, fix tests

a0f7606

More PR remarks

1659d04

maruina reviewed Jun 8, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

maruina reviewed Jun 8, 2021

View reviewed changes

maruina added 2 commits June 10, 2021 09:39

Refactor min calculation

ee37d13

Replace r.log with just log

48e7d6e

maruina force-pushed the fix-status-update branch from 41699f9 to 48e7d6e Compare June 10, 2021 15:15

maruina added 5 commits June 10, 2021 17:16

Remove resetStatus as we don't need it

70ebbd9

Semplify add/remove finalizer logic by removing the if/else loop

c083181

Rename submitStatus to updateStatusWithRetry to reflect its behaviour

99e9bf0

Replace log.V(1).Info with log.Info

63c692d

Move stage reconciliation under separate method

6760629

DiogoMCampos reviewed Jun 11, 2021

View reviewed changes

maruina mentioned this pull request Jun 14, 2021

Remove predicate.GenerationChangedPredicate{} #99

Closed

Use result.Requeue instead of a separate bool to reduce complexity

9dcee07

sledigabel reviewed Jun 14, 2021

View reviewed changes

api/v1alpha1/condition_types.go Outdated Show resolved Hide resolved

sledigabel reviewed Jun 14, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

sledigabel reviewed Jun 14, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

Remove unused StagesInitializedReason

9db2555

sledigabel approved these changes Jun 14, 2021

View reviewed changes

controllers/progressivesync_controller.go Show resolved Hide resolved

maruina merged commit adcdbca into main Jun 14, 2021

maruina deleted the fix-status-update branch August 6, 2021 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix status update #96

Fix status update #96

nebojsa-prodana commented Jun 8, 2021

maruina left a comment

maruina Jun 8, 2021

nebojsa-prodana Jun 8, 2021

lgtm-com bot commented Jun 8, 2021

nebojsa-prodana commented Jun 8, 2021

maruina Jun 8, 2021

maruina Jun 8, 2021

sledigabel commented Jun 8, 2021

DiogoMCampos Jun 11, 2021 •

edited

DiogoMCampos Jun 11, 2021

maruina Jun 14, 2021

DiogoMCampos Jun 14, 2021

DiogoMCampos Jun 11, 2021

maruina Jun 14, 2021

sledigabel left a comment

	r.Log.Error(err, "failed to update object status")
	log.Error(err, "failed to update object status")

Fix status update #96

Fix status update #96

Conversation

nebojsa-prodana commented Jun 8, 2021

maruina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 8, 2021

nebojsa-prodana commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledigabel commented Jun 8, 2021

DiogoMCampos Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledigabel left a comment

Choose a reason for hiding this comment

DiogoMCampos Jun 11, 2021 •

edited