Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require a Postgres version when creating a PostgresCluster #73

Merged
merged 2 commits into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/content/reference/pgo_create_postgrescluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ pgo create postgrescluster CLUSTER_NAME [flags]
### Examples

```
# Create a postgrescluster
pgo create postgrescluster hippo
# Create a postgrescluster with Postgres 15
pgo create postgrescluster hippo --pg-major-version 15

```
### Example output
Expand All @@ -35,7 +35,8 @@ postgresclusters/hippo created
### Options

```
-h, --help help for postgrescluster
-h, --help help for postgrescluster
--pg-major-version int Set the Postgres major version
```

### Options inherited from parent commands
Expand Down
17 changes: 11 additions & 6 deletions internal/cmd/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package cmd
import (
"context"
"fmt"
"strconv"

"github.com/spf13/cobra"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -61,8 +62,12 @@ func newCreateClusterCommand(config *internal.Config) *cobra.Command {

cmd.Args = cobra.ExactArgs(1)

cmd.Example = internal.FormatExample(`# Create a postgrescluster
pgo create postgrescluster hippo
var pgMajorVersion int
cmd.Flags().IntVar(&pgMajorVersion, "pg-major-version", 0, "Set the Postgres major version")
cobra.CheckErr(cmd.MarkFlagRequired("pg-major-version"))

cmd.Example = internal.FormatExample(`# Create a postgrescluster with Postgres 15
pgo create postgrescluster hippo --pg-major-version 15

### Example output
postgresclusters/hippo created`)
Expand All @@ -82,7 +87,7 @@ postgresclusters/hippo created`)
return err
}

cluster, err := generateUnstructuredClusterYaml(clusterName)
cluster, err := generateUnstructuredClusterYaml(clusterName, strconv.Itoa(pgMajorVersion))
if err != nil {
return err
}
Expand All @@ -104,15 +109,15 @@ postgresclusters/hippo created`)

// generateUnstructuredClusterYaml takes a name and returns a PostgresCluster
// in the unstructured format.
func generateUnstructuredClusterYaml(name string) (*unstructured.Unstructured, error) {
func generateUnstructuredClusterYaml(name, pgMajorVersion string) (*unstructured.Unstructured, error) {
var cluster unstructured.Unstructured
err := yaml.Unmarshal([]byte(fmt.Sprintf(`
apiVersion: postgres-operator.crunchydata.com/v1beta1
kind: PostgresCluster
metadata:
name: %s
spec:
postgresVersion: 14
postgresVersion: %s
instances:
- dataVolumeClaimSpec:
accessModes:
Expand All @@ -131,7 +136,7 @@ spec:
resources:
requests:
storage: 1Gi
`, name)), &cluster)
`, name, pgMajorVersion)), &cluster)

if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ spec:
resources:
requests:
storage: 1Gi
postgresVersion: 14
postgresVersion: 15
`

u, err := generateUnstructuredClusterYaml("hippo")
u, err := generateUnstructuredClusterYaml("hippo", "15")
assert.NilError(t, err)

assert.Assert(t, cmp.MarshalMatches(
Expand Down
6 changes: 4 additions & 2 deletions testing/kuttl/e2e/backup/08--backup-with-just-trigger.yaml
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: |
- script: |
pgbackrest_backup_annotation() {
kubectl get --namespace "${NAMESPACE}" postgrescluster/backup-cluster \
--output 'go-template={{ index .metadata.annotations "postgres-operator.crunchydata.com/pgbackrest-backup" }}'
}

PRIOR=$(pgbackrest_backup_annotation)
# ensure annotation timestamp will be different
sleep 1
RESULT=$(kubectl-pgo --namespace "${NAMESPACE}" backup backup-cluster)
CURRENT=$(pgbackrest_backup_annotation)

if [ "${CURRENT}" = "${PRIOR}" ]; then
printf 'Expected annotation to change, got %q' "${CURRENT}"
printf 'Expected annotation to change, got %q\n' "${CURRENT}"
exit 1
fi

Expand Down
4 changes: 2 additions & 2 deletions testing/kuttl/e2e/backup/11--backup-with-new-flag.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ commands:
echo "RESULT from taking backup: ${RESULT}"

if [ "${STATUS-0}" -eq 0 ]; then
printf 'Expected error, got none'
printf 'Expected error, got none\n'
exit 1
fi

if [[ "${RESULT,,}" != *conflict* || "${RESULT}" != *repoName* ]]; then
printf 'Expected conflict on repoName, got %q' "${RESULT}"
printf 'Expected conflict on repoName, got %q\n' "${RESULT}"
exit 1
fi
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/create/00--create_cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: kubectl-pgo --namespace $NAMESPACE create postgrescluster kuttl-create-cluster
- script: kubectl-pgo --namespace $NAMESPACE create postgrescluster --pg-major-version 15 kuttl-create-cluster
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/create/00-assert.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ spec:
requests:
storage: 1Gi
replicas: 1
postgresVersion: 14
postgresVersion: 15
status:
instances:
- name: "00"
Expand Down
46 changes: 46 additions & 0 deletions testing/kuttl/e2e/create/01--check_version_validation.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could test these not through KUTTL: https://gianarb.it/blog/golang-mockmania-cli-command-with-cobra

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, no, these errors are being returned by PGO -- the cli isn't generating anything in most of these cases, just passing through... how much "passing through" should we test?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, maybe drop the too-low/too-high tests -- that's just what we get from PGO -- and move the other tests into the unit-testing form?

(or could leave the too-low/too-high tests here: to me, we do a lot of relying on PGO in the pgo-client, and that's OK. I don't mind a belt-and-suspenders approach, where we trust but verify.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing it looks like testing this behavior elsewhere would take a good bit of effort and possibly some refactoring. I think it still may be worth dropping the 'pass-through' tests, but otherwise I think KUTTL makes the most sense for now. Happy to discuss further though if there are other options.

# Verify the error when the Postgres version given is too low.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since too low is just > 10, A user can still "create" a cluster with PG 12, even if they don't have a PG 12 available. No pods will be brought up, but for example:

> ./bin/kubectl-pgo --namespace=postgres-operator create postgrescluster elephant --pg-major-version 12
postgresclusters/elephant created
> ./bin/kubectl-pgo --namespace=postgres-operator create postgrescluster elephant --pg-major-version 13
Error: postgresclusters.postgres-operator.crunchydata.com "elephant" already exists

It seems intuitive what to do here. Just thought I'd mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Yeah, with the existing event warning we have in place, it should be fairly clear what's going on, but we can definitely try to improve this in the future, especially if we make other updates.

TOO_LOW=$(kubectl-pgo create postgrescluster --pg-major-version=1 toolow 2>&1)
if [[ "${TOO_LOW}" != "Error:"*"Invalid value"* ]]; then
printf 'Expected invalid value error, got %q\n' "${TOO_LOW}"
exit 1
fi

# Verify the error when the Postgres version given is too high.
TOO_HIGH=$(kubectl-pgo create postgrescluster --pg-major-version=100 toohigh 2>&1)
if [[ "${TOO_HIGH}" != "Error:"*"Invalid value"* ]]; then
printf 'Expected invalid value error, got %q\n' "${TOO_HIGH}"
exit 1
fi

# Verify the error when the Postgres version is not an integer.
NOT_INT=$(kubectl-pgo create postgrescluster --pg-major-version=15.1 notint 2>&1)
if [[ "${NOT_INT}" != "Error: invalid argument"* ]]; then
printf 'Expected invalid argument error, got %q\n' "${NOT_INT}"
exit 1
fi

# Verify the error when the Postgres version is not a number.
NOT_NUM=$(kubectl-pgo create postgrescluster --pg-major-version=x notnum 2>&1)
if [[ "${NOT_NUM}" != "Error: invalid argument"* ]]; then
printf 'Expected invalid argument error, got %q\n' "${NOT_NUM}"
exit 1
fi

# Verify the error when the Postgres version flag is not provided.
MISSING=$(kubectl-pgo create postgrescluster missing 2>&1)
if [[ "${MISSING}" != "Error: required flag"* ]]; then
printf 'Expected required flag error, got %q\n' "${MISSING}"
exit 1
fi

# Verify the error when the Postgres version value is empty.
NOT_SET=$(kubectl-pgo create postgrescluster --pg-major-version= notset 2>&1)
if [[ "${NOT_SET}" != "Error: invalid argument"* ]]; then
printf 'Expected invalid argument error, got %q\n' "${NOT_SET}"
exit 1
fi
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/delete-cluster/00--cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ kind: PostgresCluster
metadata:
name: delete-cluster
spec:
postgresVersion: 14
postgresVersion: 15
instances:
- name: instance1
dataVolumeClaimSpec:
Expand Down
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/restore/00--cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ kind: PostgresCluster
metadata:
name: restore-cluster
spec:
postgresVersion: 14
postgresVersion: 15
instances:
- name: instance1
dataVolumeClaimSpec:
Expand Down
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/support-export/00--create_cluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- script: kubectl-pgo --namespace $NAMESPACE create postgrescluster kuttl-support-cluster
- script: kubectl-pgo --namespace $NAMESPACE create postgrescluster --pg-major-version 15 kuttl-support-cluster
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/support-export/00-assert.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ spec:
requests:
storage: 1Gi
replicas: 1
postgresVersion: 14
postgresVersion: 15
status:
instances:
- name: "00"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ kind: PostgresCluster
metadata:
name: kuttl-support-monitoring-cluster
spec:
postgresVersion: 14
postgresVersion: 15
instances:
- dataVolumeClaimSpec:
accessModes: [ReadWriteOnce]
Expand Down
2 changes: 1 addition & 1 deletion testing/kuttl/e2e/support-export/30-assert.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ spec:
requests:
storage: 1Gi
replicas: 1
postgresVersion: 14
postgresVersion: 15
status:
instances:
- name: "00"
Expand Down
2 changes: 1 addition & 1 deletion testing/kuttl/kuttl-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ apiVersion: kuttl.dev/v1beta1
kind: TestSuite
testDirs:
- testing/kuttl/e2e/
timeout: 180
timeout: 300
parallel: 2
# by default kuttl will run in a generated namespace to override
# that functionality simply uncomment the line below and replace
Expand Down