Merge pull request #67 from SovereignCloudStack/syself/github-client-fix

🌱 create github client only if required
SovereignCloudStack · Apr 24, 2024 · 35d657a · 35d657a
2 parents 6deafac + 110e0ae
commit 35d657a
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 43 deletions.
diff --git a/internal/controller/clusterstack_controller.go b/internal/controller/clusterstack_controller.go
@@ -109,33 +109,36 @@ func (r *ClusterStackReconciler) Reconcile(ctx context.Context, req reconcile.Re
 		return reconcile.Result{}, fmt.Errorf("failed to get latest ready ClusterStackRelease: %w", err)
 	}
 
-	gc, err := r.GitHubClientFactory.NewClient(ctx)
-	if err != nil {
-		conditions.MarkFalse(clusterStack,
-			csov1alpha1.GitAPIAvailableCondition,
-			csov1alpha1.GitTokenOrEnvVariableNotSetReason,
-			clusterv1.ConditionSeverityError,
-			err.Error(),
-		)
-		record.Warnf(clusterStack, "GitTokenOrEnvVariableNotSet", err.Error())
-		return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
-	}
+	var latest *string
 
-	conditions.MarkTrue(clusterStack, csov1alpha1.GitAPIAvailableCondition)
+	if clusterStack.Spec.AutoSubscribe {
+		gc, err := r.GitHubClientFactory.NewClient(ctx)
+		if err != nil {
+			conditions.MarkFalse(clusterStack,
+				csov1alpha1.GitAPIAvailableCondition,
+				csov1alpha1.GitTokenOrEnvVariableNotSetReason,
+				clusterv1.ConditionSeverityError,
+				err.Error(),
+			)
+			record.Warnf(clusterStack, "GitTokenOrEnvVariableNotSet", err.Error())
+			return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
+		}
 
-	latest, err := getLatestReleaseFromRemoteRepository(ctx, clusterStack, gc)
-	if err != nil {
-		// only log error and mark condition as false, but continue
-		conditions.MarkFalse(clusterStack,
-			csov1alpha1.GitReleasesSyncedCondition,
-			csov1alpha1.FailedToSyncReason,
-			clusterv1.ConditionSeverityWarning,
-			err.Error(),
-		)
-		logger.Error(err, "failed to get latest release from remote repository")
-	}
+		conditions.MarkTrue(clusterStack, csov1alpha1.GitAPIAvailableCondition)
+		latest, err = getLatestReleaseFromRemoteRepository(ctx, clusterStack, gc)
+		if err != nil {
+			// only log error and mark condition as false, but continue
+			conditions.MarkFalse(clusterStack,
+				csov1alpha1.GitReleasesSyncedCondition,
+				csov1alpha1.FailedToSyncReason,
+				clusterv1.ConditionSeverityWarning,
+				err.Error(),
+			)
+			logger.Error(err, "failed to get latest release from remote repository")
+		}
 
-	conditions.MarkTrue(clusterStack, csov1alpha1.GitReleasesSyncedCondition)
+		conditions.MarkTrue(clusterStack, csov1alpha1.GitReleasesSyncedCondition)
+	}
 
 	inUse, err := r.getClusterStackReleasesInUse(ctx, req.Namespace)
 	if err != nil {
@@ -534,11 +537,6 @@ func getLatestReadyClusterStackRelease(clusterStackReleases []*csov1alpha1.Clust
 }
 
 func getLatestReleaseFromRemoteRepository(ctx context.Context, clusterStack *csov1alpha1.ClusterStack, gc githubclient.Client) (*string, error) {
-	// nothing to do if autoSubscribe is not activated
-	if !clusterStack.Spec.AutoSubscribe {
-		return nil, nil
-	}
-
 	ghReleases, resp, err := gc.ListRelease(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("failed to list releases on remote Git repository: %w", err)

diff --git a/internal/controller/clusterstackrelease_controller.go b/internal/controller/clusterstackrelease_controller.go
@@ -96,20 +96,6 @@ func (r *ClusterStackReleaseReconciler) Reconcile(ctx context.Context, req recon
 
 	controllerutil.AddFinalizer(clusterStackRelease, csov1alpha1.ClusterStackReleaseFinalizer)
 
-	gc, err := r.GitHubClientFactory.NewClient(ctx)
-	if err != nil {
-		conditions.MarkFalse(clusterStackRelease,
-			csov1alpha1.GitAPIAvailableCondition,
-			csov1alpha1.GitTokenOrEnvVariableNotSetReason,
-			clusterv1.ConditionSeverityError,
-			err.Error(),
-		)
-		record.Warnf(clusterStackRelease, "GitTokenOrEnvVariableNotSet", err.Error())
-		return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
-	}
-
-	conditions.MarkTrue(clusterStackRelease, csov1alpha1.GitAPIAvailableCondition)
-
 	// name of ClusterStackRelease object is same as the release tag
 	releaseTag := clusterStackRelease.Name
 
@@ -127,6 +113,20 @@ func (r *ClusterStackReleaseReconciler) Reconcile(ctx context.Context, req recon
 		// acquire lock so that only one reconcile loop can download the release
 		r.clusterStackRelDownloadDirectoryMutex.Lock()
 
+		gc, err := r.GitHubClientFactory.NewClient(ctx)
+		if err != nil {
+			conditions.MarkFalse(clusterStackRelease,
+				csov1alpha1.GitAPIAvailableCondition,
+				csov1alpha1.GitTokenOrEnvVariableNotSetReason,
+				clusterv1.ConditionSeverityError,
+				err.Error(),
+			)
+			record.Warnf(clusterStackRelease, "GitTokenOrEnvVariableNotSet", err.Error())
+			return reconcile.Result{}, fmt.Errorf("failed to create Github client: %w", err)
+		}
+
+		conditions.MarkTrue(clusterStackRelease, csov1alpha1.GitAPIAvailableCondition)
+
 		if err := downloadReleaseAssets(ctx, releaseTag, releaseAssets.LocalDownloadPath, gc); err != nil {
 			return reconcile.Result{}, fmt.Errorf("failed to download release assets: %w", err)
 		}