Remove reconcilePGAdminVersion, adjust tests and add clarifying comments

Author: tjmoore4

internal/controller/standalone_pgadmin/users.go

@@ -86,11 +86,21 @@ func (r *PGAdminReconciler) reconcilePGAdminUsers(ctx context.Context, pgadmin *
 	if pgadmin.Status.MajorVersion == 0 || pgadmin.Status.MinorVersion == "" ||
 		pgadmin.Status.ImageSHA != pgAdminImageSha {
 
-		pgadminMinorVersion, err := r.reconcilePGAdminVersion(ctx, podExecutor)
-		if err != nil {
+		// exec into the pgAdmin pod and retrieve the pgAdmin minor version
+		script := fmt.Sprintf(`
+PGADMIN_DIR=%s
+cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_VERSION)"
+`, pgAdminDir)
+
+		var stdin, stdout, stderr bytes.Buffer
+
+		if err := podExecutor(ctx, &stdin, &stdout, &stderr,
+			[]string{"bash", "-ceu", "--", script}...); err != nil {
 			return err
 		}
 
+		pgadminMinorVersion := strings.TrimSpace(stdout.String())
+
 		// ensure minor version is valid before storing in status
 		parsedMinorVersion, err := strconv.ParseFloat(pgadminMinorVersion, 64)
 		if err != nil {
@@ -124,25 +134,6 @@ func (r *PGAdminReconciler) reconcilePGAdminUsers(ctx context.Context, pgadmin *
 	return r.writePGAdminUsers(ctx, pgadmin, podExecutor)
 }
 
-// reconcilePGAdminVersion execs into the pgAdmin pod and retrieves the pgAdmin minor version
-func (r *PGAdminReconciler) reconcilePGAdminVersion(ctx context.Context, exec Executor) (string, error) {
-	script := fmt.Sprintf(`
-PGADMIN_DIR=%s
-cd $PGADMIN_DIR && python3 -c "import config; print(config.APP_VERSION)"
-`, pgAdminDir)
-
-	var stdin, stdout, stderr bytes.Buffer
-
-	err := exec(ctx, &stdin, &stdout, &stderr,
-		[]string{"bash", "-ceu", "--", script}...)
-
-	if err != nil {
-		return "", err
-	}
-
-	return strings.TrimSpace(stdout.String()), nil
-}
-
 // writePGAdminUsers takes the users in the pgAdmin spec and writes (adds or updates) their data
 // to both pgAdmin and the users.json file that is stored in the pgAdmin secret. If a user is
 // removed from the spec, its data is removed from users.json, but it is not deleted from pgAdmin.
@@ -188,7 +179,7 @@ cd $PGADMIN_DIR
 	}
 
 	var olderThan9_3 bool
-	versionFloat, err := strconv.ParseFloat(pgadmin.Status.MinorVersion, 32)
+	versionFloat, err := strconv.ParseFloat(pgadmin.Status.MinorVersion, 64)
 	if err != nil {
 		return err
 	}
@@ -199,6 +190,8 @@ cd $PGADMIN_DIR
 	intentUsers := []pgAdminUserForJson{}
 	for _, user := range pgadmin.Spec.Users {
 		var stdin, stdout, stderr bytes.Buffer
+		// starting in pgAdmin 9.3, custom roles are supported and a new flag is used
+		// - https://github.com/pgadmin-org/pgadmin4/pull/8631
 		typeFlag := "--role User"
 		if olderThan9_3 {
 			typeFlag = "--nonadmin"
@@ -258,10 +251,13 @@ cd $PGADMIN_DIR
 					log.Error(err, "PodExec failed: ")
 					intentUsers = append(intentUsers, existingUser)
 					continue
+
 				} else if strings.Contains(strings.TrimSpace(stderr.String()), "UserWarning: pkg_resources is deprecated as an API") {
+					// Started seeing this error with pgAdmin 9.7 when using Python 3.11.
+					// Issue appears to resolve with Python 3.13.
 					log.Info(stderr.String())
 				} else if strings.TrimSpace(stderr.String()) != "" {
-					log.Error(errors.New(stderr.String()), fmt.Sprintf("pgAdmin setup.py error for %s: ",
+					log.Error(errors.New(stderr.String()), fmt.Sprintf("pgAdmin setup.py update-user error for %s: ",
 						intentUser.Username))
 					intentUsers = append(intentUsers, existingUser)
 					continue
@@ -295,9 +291,11 @@ cd $PGADMIN_DIR
 				continue
 			}
 			if strings.Contains(strings.TrimSpace(stderr.String()), "UserWarning: pkg_resources is deprecated as an API") {
+				// Started seeing this error with pgAdmin 9.7 when using Python 3.11.
+				// Issue appears to resolve with Python 3.13.
 				log.Info(stderr.String())
 			} else if strings.TrimSpace(stderr.String()) != "" {
-				log.Error(errors.New(stderr.String()), fmt.Sprintf("pgAdmin setup.py error for %s: ",
+				log.Error(errors.New(stderr.String()), fmt.Sprintf("pgAdmin setup.py add-user error for %s: ",
 					intentUser.Username))
 				continue
 			}

internal/controller/standalone_pgadmin/users_test.go

@@ -195,52 +195,40 @@ func TestReconcilePGAdminUsers(t *testing.T) {
 		assert.Equal(t, pgadmin.Status.MinorVersion, "")
 		assert.Equal(t, pgadmin.Status.ImageSHA, "")
 	})
-}
 
-func TestReconcilePGAdminVersion(t *testing.T) {
-	ctx := context.Background()
-	pod := corev1.Pod{}
-	pod.Namespace = "test-namespace"
-	pod.Name = "pgadmin-123-0"
-	reconciler := &PGAdminReconciler{}
+	t.Run("PodExecError", func(t *testing.T) {
+		pgadmin := pgadmin.DeepCopy()
+		pod := pod.DeepCopy()
 
-	podExecutor := func(
-		ctx context.Context, stdin io.Reader, stdout, stderr io.Writer, command ...string,
-	) error {
-		return reconciler.PodExec(ctx, pod.Namespace, pod.Name, "pgadmin", stdin, stdout, stderr, command...)
-	}
+		pod.DeletionTimestamp = nil
+		pod.Status.ContainerStatuses =
+			[]corev1.ContainerStatus{{Name: naming.ContainerPGAdmin}}
+		pod.Status.ContainerStatuses[0].State.Running =
+			new(corev1.ContainerStateRunning)
+		pod.Status.ContainerStatuses[0].ImageID = "fakeSHA"
 
-	t.Run("SuccessfulRetrieval", func(t *testing.T) {
-		reconciler.PodExec = func(
+		r := new(PGAdminReconciler)
+		r.Reader = fake.NewClientBuilder().WithObjects(pod).Build()
+
+		calls := 0
+		r.PodExec = func(
 			ctx context.Context, namespace, pod, container string,
 			stdin io.Reader, stdout, stderr io.Writer, command ...string,
 		) error {
+			calls++
+
 			assert.Equal(t, pod, "pgadmin-123-0")
-			assert.Equal(t, namespace, "test-namespace")
+			assert.Equal(t, namespace, pgadmin.Namespace)
 			assert.Equal(t, container, naming.ContainerPGAdmin)
 
-			// Simulate a v9.3 version of pgAdmin by setting stdout to "9.3"
-			// for podexec call in reconcilePGAdminVersion
-			_, _ = stdout.Write([]byte("9.3"))
-			return nil
-		}
-
-		version, err := reconciler.reconcilePGAdminVersion(ctx, podExecutor)
-		assert.NilError(t, err)
-		assert.Equal(t, version, "9.3")
-	})
-
-	t.Run("PodExecError", func(t *testing.T) {
-		reconciler.PodExec = func(
-			ctx context.Context, namespace, pod, container string,
-			stdin io.Reader, stdout, stderr io.Writer, command ...string,
-		) error {
 			return errors.New("PodExecError")
 		}
 
-		version, err := reconciler.reconcilePGAdminVersion(ctx, podExecutor)
-		assert.Check(t, err != nil)
-		assert.Equal(t, version, "")
+		assert.Error(t, r.reconcilePGAdminUsers(ctx, pgadmin), "PodExecError")
+		assert.Equal(t, calls, 1, "PodExec should be called once")
+		assert.Equal(t, pgadmin.Status.MajorVersion, 0)
+		assert.Equal(t, pgadmin.Status.MinorVersion, "")
+		assert.Equal(t, pgadmin.Status.ImageSHA, "")
 	})
 }