Fix subtests running in same environment and reconciliation bug which was not covered because of that (#602)

Author: AndiDog

exp/controllers/awsmachinepool_controller_test.go

@@ -489,15 +489,15 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 		})
 
 		t.Run("ReconcileLaunchTemplate not mocked", func(t *testing.T) {
-			g := NewWithT(t)
-			setup(t, g)
-			reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
-			reconSvc = nil                           // not used
-			defer teardown(t, g)
-
 			launchTemplateIDExisting := "lt-existing"
 
 			t.Run("nothing exists, so launch template and ASG must be created", func(t *testing.T) {
+				g := NewWithT(t)
+				setup(t, g)
+				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
+				reconSvc = nil                           // not used
+				defer teardown(t, g)
+
 				ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return(nil, "", nil, nil, nil)
 				ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(ptr.To[string]("ami-abcdef123"), nil)
 				ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(ptr.To[string]("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script")), gomock.Eq(userdata.ComputeHash([]byte("shell-script")))).Return("lt-ghijkl456", nil)
@@ -514,6 +514,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			})
 
 			t.Run("launch template and ASG exist and need no update", func(t *testing.T) {
+				g := NewWithT(t)
+				setup(t, g)
+				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
+				reconSvc = nil                           // not used
+				defer teardown(t, g)
+
 				// Latest ID and version already stored, no need to retrieve it
 				ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting
 				ms.AWSMachinePool.Status.LaunchTemplateVersion = ptr.To[string]("1")
@@ -556,6 +562,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			})
 
 			t.Run("launch template and ASG exist and only AMI ID changed", func(t *testing.T) {
+				g := NewWithT(t)
+				setup(t, g)
+				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
+				reconSvc = nil                           // not used
+				defer teardown(t, g)
+
 				// Latest ID and version already stored, no need to retrieve it
 				ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting
 				ms.AWSMachinePool.Status.LaunchTemplateVersion = ptr.To[string]("1")
@@ -604,6 +616,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			})
 
 			t.Run("launch template and ASG exist and only bootstrap data secret name changed", func(t *testing.T) {
+				g := NewWithT(t)
+				setup(t, g)
+				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
+				reconSvc = nil                           // not used
+				defer teardown(t, g)
+
 				// Latest ID and version already stored, no need to retrieve it
 				ms.AWSMachinePool.Status.LaunchTemplateID = launchTemplateIDExisting
 				ms.AWSMachinePool.Status.LaunchTemplateVersion = ptr.To[string]("1")
@@ -655,6 +673,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 			})
 
 			t.Run("launch template and ASG created from zero, then bootstrap config reference changes", func(t *testing.T) {
+				g := NewWithT(t)
+				setup(t, g)
+				reconciler.reconcileServiceFactory = nil // use real implementation, but keep EC2 calls mocked (`ec2ServiceFactory`)
+				reconSvc = nil                           // not used
+				defer teardown(t, g)
+
 				ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return(nil, "", nil, nil, nil)
 				ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(ptr.To[string]("ami-abcdef123"), nil)
 				ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(ptr.To[string]("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script")), gomock.Eq(userdata.ComputeHash([]byte("shell-script")))).Return("lt-ghijkl456", nil)
@@ -685,6 +709,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 				g.Expect(testEnv.Create(ctx, newBootstrapSecret)).To(Succeed())
 				ms.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To[string](newBootstrapSecret.Name)
 
+				// Since `AWSMachinePool.status.launchTemplateVersion` isn't set yet,
+				// the controller will ask for the current version and then set the status.
+				ec2Svc.EXPECT().GetLaunchTemplateLatestVersion(gomock.Any()).Return("1", nil)
+
 				ec2Svc.EXPECT().GetLaunchTemplate(gomock.Eq("test")).Return(
 					&expinfrav1.AWSLaunchTemplate{
 						Name: "test",

pkg/cloud/services/ec2/launchtemplate.go

@@ -202,7 +202,9 @@ func (s *Service) ReconcileLaunchTemplate(
 			return nil, err
 		}
 		scope.SetLaunchTemplateLatestVersionStatus(launchTemplateVersion)
-		return nil, scope.PatchObject()
+		if err = scope.PatchObject(); err != nil {
+			return nil, err
+		}
 	}
 
 	annotation, err := MachinePoolAnnotationJSON(scope, TagsLastAppliedAnnotation)