Fix subtests running in same environment and reconciliation bug which…

… was not covered because of that (#602)
giantswarm · Jul 11, 2024 · 89a59a6 · 89a59a6
1 parent d64510b
commit 89a59a6
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 8 deletions.
diff --git a/exp/controllers/awsmachinepool_controller_test.go b/exp/controllers/awsmachinepool_controller_test.go
@@ -472,15 +472,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)
 				ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(pointer.String("ami-abcdef123"), nil)
 				ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(pointer.String("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script"))).Return("lt-ghijkl456", nil)
@@ -497,6 +497,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 = pointer.String("1")
@@ -538,6 +544,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 = pointer.String("1")
@@ -585,6 +597,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 = pointer.String("1")
@@ -635,6 +653,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)
 				ec2Svc.EXPECT().DiscoverLaunchTemplateAMI(gomock.Any()).Return(pointer.String("ami-abcdef123"), nil)
 				ec2Svc.EXPECT().CreateLaunchTemplate(gomock.Any(), gomock.Eq(pointer.String("ami-abcdef123")), gomock.Eq(userDataSecretKey), gomock.Eq([]byte("shell-script"))).Return("lt-ghijkl456", nil)
@@ -650,7 +674,6 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 				g.Expect(err).To(Succeed())
 
 				g.Expect(ms.AWSMachinePool.Status.LaunchTemplateID).ToNot(BeEmpty())
-				g.Expect(pointer.StringDeref(ms.AWSMachinePool.Status.LaunchTemplateVersion, "")).ToNot(BeEmpty())
 
 				// Data secret name changes
 				newBootstrapSecret := &corev1.Secret{
@@ -665,6 +688,10 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
 				g.Expect(testEnv.Create(ctx, newBootstrapSecret)).To(Succeed())
 				ms.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName = pointer.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",

diff --git a/pkg/cloud/services/ec2/launchtemplate.go b/pkg/cloud/services/ec2/launchtemplate.go
@@ -199,7 +199,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)