Merge pull request #538 from darkdoc/fix-cardill-issue

fix cardill issue

Author: openshift-merge-bot[bot]

internal/controller/argo.go

@@ -411,20 +411,31 @@ func createOrUpdateArgoCD(client dynamic.Interface, fullClient kubernetes.Interf
 
 	if !haveArgo(client, name, namespace) {
 		// create it
-		obj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(argo)
+		obj, errConvert := runtime.DefaultUnstructuredConverter.ToUnstructured(argo)
+		if errConvert != nil {
+			return fmt.Errorf("failed to convert ArgoCD to unstructured for create: %v", errConvert)
+		}
 		newArgo := &unstructured.Unstructured{Object: obj}
 		_, err = client.Resource(gvr).Namespace(namespace).Create(context.TODO(), newArgo, metav1.CreateOptions{})
 	} else { // update it
-		oldArgo, _ := getArgoCD(client, name, namespace)
+		oldArgo, errGet := getArgoCDFunc(client, name, namespace)
+		if errGet != nil {
+			return fmt.Errorf("failed to get existing ArgoCD %s/%s: %v", namespace, name, errGet)
+		}
 		argo.SetResourceVersion(oldArgo.GetResourceVersion())
-		obj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(argo)
+		obj, errConvert := runtime.DefaultUnstructuredConverter.ToUnstructured(argo)
+		if errConvert != nil {
+			return fmt.Errorf("failed to convert ArgoCD to unstructured for update: %v", errConvert)
+		}
 		newArgo := &unstructured.Unstructured{Object: obj}
 
 		_, err = client.Resource(gvr).Namespace(namespace).Update(context.TODO(), newArgo, metav1.UpdateOptions{})
 	}
 	return err
 }
 
+var getArgoCDFunc = getArgoCD
+
 func getArgoCD(client dynamic.Interface, name, namespace string) (*argooperator.ArgoCD, error) {
 	gvr := schema.GroupVersionResource{Group: ArgoCDGroup, Version: ArgoCDVersion, Resource: ArgoCDResource}
 	argo := &argooperator.ArgoCD{}
@@ -705,9 +716,10 @@ func commonApplicationSourceHelm(p *api.Pattern, prefix string) *argoapi.Applica
 	valueFiles := newApplicationValueFiles(p, prefix)
 	sharedValueFiles, err := getSharedValueFiles(p, prefix)
 	if err != nil {
-		fmt.Printf("Could not fetch sharedValueFiles: %s", err)
+		log.Printf("Could not fetch sharedValueFiles: %s", err)
+	} else {
+		valueFiles = append(valueFiles, sharedValueFiles...)
 	}
-	valueFiles = append(valueFiles, sharedValueFiles...)
 
 	return &argoapi.ApplicationSourceHelm{
 		ValueFiles: valueFiles,
@@ -910,9 +922,12 @@ func getApplication(client argoclient.Interface, name, namespace string) (*argoa
 
 func createApplication(client argoclient.Interface, app *argoapi.Application, namespace string) error {
 	saved, err := client.ArgoprojV1alpha1().Applications(namespace).Create(context.Background(), app, metav1.CreateOptions{})
+	if err != nil {
+		return err
+	}
 	yamlOutput, _ := objectYaml(saved)
 	log.Printf("Created: %s\n", yamlOutput)
-	return err
+	return nil
 }
 
 func updateApplication(client argoclient.Interface, target, current *argoapi.Application, namespace string) (bool, error) {

internal/controller/argo_test.go

@@ -998,6 +998,40 @@ var _ = Describe("CreateOrUpdateArgoCD", func() {
 			Expect(argoCD.GetResourceVersion()).To(Equal("1")) // Ensure it has been updated
 		})
 	})
+
+	Context("when there is an error in the getArgoCD fn but there is an argocd", func() {
+
+		BeforeEach(func() {
+			argoCD := &unstructured.Unstructured{
+				Object: map[string]any{
+					"apiVersion": "argoproj.io/v1beta1",
+					"kind":       "ArgoCD",
+					"metadata": map[string]any{
+						"name":            name,
+						"namespace":       namespace,
+						"resourceVersion": "1",
+					},
+				},
+			}
+			_, err := dynamicClient.Resource(gvr).Namespace(namespace).Create(context.TODO(), argoCD, metav1.CreateOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			getArgoCDFunc = func(_ dynamic.Interface, _, _ string) (*argooperator.ArgoCD, error) {
+				return nil, fmt.Errorf("forced error")
+			}
+		})
+
+		It("should propagate the error and not update the existing argocd", func() {
+			err := createOrUpdateArgoCD(dynamicClient, nil, name, namespace)
+			Expect(err).To(HaveOccurred())
+
+			argoCD, err := dynamicClient.Resource(gvr).Namespace(namespace).Get(context.TODO(), name, metav1.GetOptions{})
+
+			Expect(err).NotTo(HaveOccurred())
+			Expect(argoCD).ToNot(BeNil())
+			Expect(argoCD.GetResourceVersion()).To(Equal("1"))
+
+		})
+	})
 })
 
 var _ = Describe("CompareApplication", func() {

internal/controller/pattern_controller.go

@@ -599,7 +599,11 @@ func (r *PatternReconciler) deleteSpokeApps(targetApp, app *argoapi.Application,
 	log.Printf("Deletion phase: %s - checking if all child applications are gone from spoke", api.DeleteSpokeChildApps)
 
 	// Update application with deletePattern=DeleteSpokeChildApps to trigger spoke child deletion
-	if changed, _ := updateApplication(r.argoClient, targetApp, app, namespace); changed {
+	changed, errUpdate := updateApplication(r.argoClient, targetApp, app, namespace)
+	if errUpdate != nil {
+		return fmt.Errorf("failed to update application %q for spoke child deletion: %v", app.Name, errUpdate)
+	}
+	if changed {
 		return fmt.Errorf("updated application %q for spoke child deletion", app.Name)
 	}
 
@@ -660,7 +664,11 @@ func (r *PatternReconciler) deleteHubApps(targetApp, app *argoapi.Application, n
 	}
 
 	// Update application with deletePattern=DeleteHubChildApps to trigger hub child app deletion
-	if changed, _ := updateApplication(r.argoClient, targetApp, app, namespace); changed {
+	changed, errUpdate := updateApplication(r.argoClient, targetApp, app, namespace)
+	if errUpdate != nil {
+		return fmt.Errorf("failed to update application %q for hub deletion: %v", app.Name, errUpdate)
+	}
+	if changed {
 		return fmt.Errorf("updated application %q for hub deletion", app.Name)
 	}
 
@@ -731,7 +739,11 @@ func (r *PatternReconciler) finalizeObject(instance *api.Pattern) error {
 
 		// Phase 2: Delete app of apps from spoke
 		if qualifiedInstance.Status.DeletionPhase == api.DeleteSpoke {
-			if changed, _ := updateApplication(r.argoClient, targetApp, app, ns); changed {
+			changed, errUpdate := updateApplication(r.argoClient, targetApp, app, ns)
+			if errUpdate != nil {
+				return fmt.Errorf("failed to update application %q for spoke app of apps deletion: %v", app.Name, errUpdate)
+			}
+			if changed {
 				return fmt.Errorf("updated application %q for spoke app of apps deletion", app.Name)
 			}