Fix stack state message regression #107
Conversation
| if !isStackMarkedToBeDeleted && (instance.Status.LastUpdate != nil && instance.Status.LastUpdate.State == instance.Spec.Commit) { | ||
| reqLogger.Info("Stack already at desired state", "Stack.Commit", instance.Spec.Commit) | ||
| return reconcile.Result{}, nil | ||
| } |
There was a problem hiding this comment.
What was the original argument for short-circuiting here? I believe it's correct to not do this, but not sure exactly why it was added and thus not sure what potentially undesirable consequence this might have?
There was a problem hiding this comment.
I am not really sure on this unfortunately. One theory I have is that this was an attempt at avoiding reconciliation loops (modifying the stack CR in the reconciliation loop causes more reconciliations to trigger). #29 introduced this change and was also intended to address the reconciliation loop issue. I am running the integration tests which apparently reproduced the issues so I am hoping we can build confidence with them before pushing this change.
| Permalink: permalink, | ||
| LastAttemptedCommit: instance.Spec.Commit, | ||
| State: pulumiv1alpha1.FailedStackStateMessage, | ||
| Permalink: permalink, |
There was a problem hiding this comment.
Will this result in overwriting lastSucceasfulCommit to nil if it was present?
There was a problem hiding this comment.
Yes. Thanks for pointing out. Updated now.
| if err != nil { | ||
| return errors.Wrapf(err, "failed to determine revision for git repository at %s", sess.workdir) | ||
| } | ||
| sess.currentCommit = headRef.Hash().String() |
There was a problem hiding this comment.
The way we currently do all this mutation of sess through these various methods definitely feels a little brittle - though I recognize you are continuing with the pattern that already largely exists here. Not sure it's worth it for a one-off here - but I could imagine this function just returning the commit (and not even being a method on the session), just to make the action-at-a-distance aspect a little less bad here.
There was a problem hiding this comment.
Related - if we aren't going to do that - I would rename this to setCurrentCommit or something similar to be more clear on what the function does.
There was a problem hiding this comment.
I agree with your original statement. In general the the mutative changes are a bit heavy here. I went with the approach you outlined in your former comment.
| sess.logger.Error(err, "Failed to delete working dir: %s", sess.workdir) | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
Minor - I wonder if we should move this into a sess.ClosePulumiWorkdir method and put that near SetupPulumiWorkdir to make the creation/destruction more cleanly coupled?
test/stack_controller_test.go
Outdated
| @@ -216,7 +216,7 @@ var _ = Describe("Stack Controller", func() { | |||
| return false | |||
| } | |||
| if fetched.Status.LastUpdate != nil { | |||
| return fetched.Status.LastUpdate.State == commit | |||
| return fetched.Status.LastUpdate.LastSuccessfulCommit == commit | |||
There was a problem hiding this comment.
Are there tests we can add that cover state transitions for lastAttemptedCommit and state as well?
| @@ -396,25 +388,6 @@ func newReconcileStackSession( | |||
| } | |||
| } | |||
|
|
|||
| // gitCloneAndCheckoutCommit clones the Git repository and checkouts the specified commit hash or branch. | |||
| func gitCloneAndCheckoutCommit(url, hash, branch, path string) error { | |||
There was a problem hiding this comment.
Is this handled elsewhere now?
There was a problem hiding this comment.
Yes - its done by the automation api so this was unused. See auth.NewLocalWorkspace.
Fixes #102 and #98 which was caused due to what appears to be a regression in functionality introduced in #29.
We now record the last attempted and successful commits explicitly in their own fields in the
StackUpdateStatesubresource under stack.The original intent of reusing
StackUpdateState.Statefor success and commit recording isn't clear. I have run the integration test suite locally multiple times with success which allays my fears about introducing this change given that #29 was merged to address issues demonstrated by the test suite.In addition, we were never deleting the working directories and creating a new workspace for each reconciliation loop which resulted in #104. I considered retaining the workspace for the lifetime of the stack which might help avoid pulling down dependencies etc. multiple times. However, recreating the workspace every time seems to be more desirable than tying the lifecycle of the workspace/working directory to the stack (for determinism/reproducibility). As a result, we now delete the workspace working directory after each reconciliation - see 8bc2723. While the most desirable fix for this will come through #78, this should have a significant impact in the meantime.
Note that CI integration is currently failing in this repo due to recent deprecations of certain GHA commands. Working to fix those separately.