incorporate review comments

Signed-off-by: Ashutosh Kumar <[email protected]>

azure/defaults.go

@@ -81,7 +81,9 @@ const (
 const (
 	// ProviderIDPrefix will be appended to the beginning of Azure resource IDs to form the Kubernetes Provider ID.
 	// NOTE: this format matches the 2 slashes format used in cloud-provider and cluster-autoscaler.
-	ProviderIDPrefix          = "azure://"
+	ProviderIDPrefix = "azure://"
+	// azureBuiltInContributorID the ID of the Contributor role in Azure
+	// Ref: https://docs.microsoft.com/en-us/azure/role-based-access-control/built-in-roles
 	azureBuiltInContributorID = "b24988ac-6180-42a0-ab88-20f7382dd24c"
 )
 

azure/services/roleassignments/client.go

@@ -95,7 +95,7 @@ func (ac *azureClient) Result(ctx context.Context, futureData azureautorest.Futu
 	return nil, nil
 }
 
+// DeleteAsync is no-op for role assignments. It gets deleted as part of the VM deletion.
 func (ac *azureClient) DeleteAsync(ctx context.Context, spec azure.ResourceSpecGetter) (azureautorest.FutureAPI, error) {
-	// ToDo: Complete this function
 	return nil, nil
 }

azure/services/scalesets/client.go

@@ -161,7 +161,7 @@ func (ac *AzureClient) CreateOrUpdateAsync(ctx context.Context, resourceGroupNam
 	if err != nil {
 		// if an error occurs, return the future.
 		// this means the long-running operation didn't finish in the specified timeout.
-		return converters.SDKToFuture(&future, infrav1.PutFuture, ScalesetsServiceName, vmssName, resourceGroupName)
+		return converters.SDKToFuture(&future, infrav1.PutFuture, servicename, vmssName, resourceGroupName)
 	}
 
 	// todo: this returns the result VMSS, we should use it
@@ -194,7 +194,7 @@ func (ac *AzureClient) UpdateAsync(ctx context.Context, resourceGroupName, vmssN
 	if err != nil {
 		// if an error occurs, return the future.
 		// this means the long-running operation didn't finish in the specified timeout.
-		return converters.SDKToFuture(&future, infrav1.PatchFuture, ScalesetsServiceName, vmssName, resourceGroupName)
+		return converters.SDKToFuture(&future, infrav1.PatchFuture, servicename, vmssName, resourceGroupName)
 	}
 	// todo: this returns the result VMSS, we should use it
 	_, err = future.Result(ac.scalesets)
@@ -305,7 +305,7 @@ func (ac *AzureClient) DeleteAsync(ctx context.Context, resourceGroupName, vmssN
 	if err != nil {
 		// if an error occurs, return the future.
 		// this means the long-running operation didn't finish in the specified timeout.
-		return converters.SDKToFuture(&future, infrav1.DeleteFuture, ScalesetsServiceName, vmssName, resourceGroupName)
+		return converters.SDKToFuture(&future, infrav1.DeleteFuture, servicename, vmssName, resourceGroupName)
 	}
 	_, err = future.Result(ac.scalesets)
 

azure/services/scalesets/scalesets.go

@@ -58,8 +58,8 @@ type (
 	}
 )
 
-// ScalesetsServiceName is the name of the scalesets service.
-const ScalesetsServiceName = "scalesets"
+// servicename is the name of the scalesets service.
+const servicename = "scalesets"
 
 // NewService creates a new service.
 func NewService(scope ScaleSetScope, skuCache *resourceskus.Cache) *Service {
@@ -86,7 +86,7 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) {
 
 	// check if there is an ongoing long running operation
 	var (
-		future      = s.Scope.GetLongRunningOperationState(s.Scope.ScaleSetSpec().Name, ScalesetsServiceName)
+		future      = s.Scope.GetLongRunningOperationState(s.Scope.ScaleSetSpec().Name, servicename)
 		fetchedVMSS *azure.VMSS
 	)
 
@@ -141,7 +141,7 @@ func (s *Service) Reconcile(ctx context.Context) (retErr error) {
 	}
 
 	// if we get to here, we have completed any long running VMSS operations (creates / updates)
-	s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, ScalesetsServiceName)
+	s.Scope.DeleteLongRunningOperationState(s.Scope.ScaleSetSpec().Name, servicename)
 	return nil
 }
 
@@ -168,7 +168,7 @@ func (s *Service) Delete(ctx context.Context) error {
 	}()
 
 	// check if there is an ongoing long running operation
-	future := s.Scope.GetLongRunningOperationState(vmssSpec.Name, ScalesetsServiceName)
+	future := s.Scope.GetLongRunningOperationState(vmssSpec.Name, servicename)
 	if future != nil {
 		// if the operation is not complete this will return an error
 		_, err := s.GetResultIfDone(ctx, future)
@@ -177,7 +177,7 @@ func (s *Service) Delete(ctx context.Context) error {
 		}
 
 		// ScaleSet has been deleted
-		s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, ScalesetsServiceName)
+		s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, servicename)
 		return nil
 	}
 
@@ -201,7 +201,7 @@ func (s *Service) Delete(ctx context.Context) error {
 	}
 
 	// future is either nil, or the result of the future is complete
-	s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, ScalesetsServiceName)
+	s.Scope.DeleteLongRunningOperationState(vmssSpec.Name, servicename)
 	return nil
 }
 

azure/services/scalesets/scalesets_test.go

@@ -220,7 +220,7 @@ func TestReconcileVMSS(t *testing.T) {
 				instances := newDefaultInstances()
 
 				setupDefaultVMSSInProgressOperationDoneExpectations(s, m, createdVMSS, instances)
-				s.DeleteLongRunningOperationState(defaultSpec.Name, ScalesetsServiceName)
+				s.DeleteLongRunningOperationState(defaultSpec.Name, servicename)
 			},
 		},
 		{
@@ -233,7 +233,7 @@ func TestReconcileVMSS(t *testing.T) {
 				instances := newDefaultInstances()
 
 				setupDefaultVMSSInProgressOperationDoneExpectations(s, m, createdVMSS, instances)
-				s.DeleteLongRunningOperationState(defaultSpec.Name, ScalesetsServiceName)
+				s.DeleteLongRunningOperationState(defaultSpec.Name, servicename)
 			},
 		},
 		{
@@ -571,11 +571,11 @@ func TestDeleteVMSS(t *testing.T) {
 				}).AnyTimes()
 				s.ResourceGroup().AnyTimes().Return("my-existing-rg")
 				future := &infrav1.Future{}
-				s.GetLongRunningOperationState("my-existing-vmss", ScalesetsServiceName).Return(future)
+				s.GetLongRunningOperationState("my-existing-vmss", servicename).Return(future)
 				m.GetResultIfDone(gomockinternal.AContext(), future).Return(compute.VirtualMachineScaleSet{}, nil)
 				m.Get(gomockinternal.AContext(), "my-existing-rg", "my-existing-vmss").
 					Return(compute.VirtualMachineScaleSet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
-				s.DeleteLongRunningOperationState("my-existing-vmss", ScalesetsServiceName)
+				s.DeleteLongRunningOperationState("my-existing-vmss", servicename)
 			},
 		},
 		{
@@ -588,7 +588,7 @@ func TestDeleteVMSS(t *testing.T) {
 					Capacity: 3,
 				}).AnyTimes()
 				s.ResourceGroup().AnyTimes().Return(resourceGroup)
-				s.GetLongRunningOperationState(name, ScalesetsServiceName).Return(nil)
+				s.GetLongRunningOperationState(name, servicename).Return(nil)
 				m.DeleteAsync(gomockinternal.AContext(), resourceGroup, name).
 					Return(nil, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 				m.Get(gomockinternal.AContext(), resourceGroup, name).
@@ -605,7 +605,7 @@ func TestDeleteVMSS(t *testing.T) {
 					Capacity: 3,
 				}).AnyTimes()
 				s.ResourceGroup().AnyTimes().Return(resourceGroup)
-				s.GetLongRunningOperationState(name, ScalesetsServiceName).Return(nil)
+				s.GetLongRunningOperationState(name, servicename).Return(nil)
 				m.DeleteAsync(gomockinternal.AContext(), resourceGroup, name).
 					Return(nil, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 500}, "Internal Server Error"))
 				m.Get(gomockinternal.AContext(), resourceGroup, name).
@@ -1159,7 +1159,7 @@ func setupDefaultVMSSInProgressOperationDoneExpectations(s *mock_scalesets.MockS
 		Name:          defaultVMSSName,
 		Data:          "",
 	}
-	s.GetLongRunningOperationState(defaultVMSSName, ScalesetsServiceName).Return(future)
+	s.GetLongRunningOperationState(defaultVMSSName, servicename).Return(future)
 	m.GetResultIfDone(gomockinternal.AContext(), future).Return(createdVMSS, nil).AnyTimes()
 	m.ListInstances(gomockinternal.AContext(), defaultResourceGroup, defaultVMSSName).Return(instances, nil).AnyTimes()
 	s.MaxSurge().Return(1, nil)
@@ -1169,7 +1169,7 @@ func setupDefaultVMSSInProgressOperationDoneExpectations(s *mock_scalesets.MockS
 
 func setupDefaultVMSSStartCreatingExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder, m *mock_scalesets.MockClientMockRecorder) {
 	setupDefaultVMSSExpectations(s)
-	s.GetLongRunningOperationState(defaultVMSSName, ScalesetsServiceName).Return(nil)
+	s.GetLongRunningOperationState(defaultVMSSName, servicename).Return(nil)
 	m.Get(gomockinternal.AContext(), defaultResourceGroup, defaultVMSSName).
 		Return(compute.VirtualMachineScaleSet{}, autorest.NewErrorWithResponse("", "", &http.Response{StatusCode: 404}, "Not found"))
 }
@@ -1234,7 +1234,7 @@ func setupVMSSExpectationsWithoutVMImage(s *mock_scalesets.MockScaleSetScopeMock
 func setupDefaultVMSSUpdateExpectations(s *mock_scalesets.MockScaleSetScopeMockRecorder) {
 	setupUpdateVMSSExpectations(s)
 	s.SetProviderID(azure.ProviderIDPrefix + "vmss-id")
-	s.GetLongRunningOperationState(defaultVMSSName, ScalesetsServiceName).Return(nil)
+	s.GetLongRunningOperationState(defaultVMSSName, servicename).Return(nil)
 	s.MaxSurge().Return(1, nil)
 	s.SetVMSSState(gomock.Any())
 }