ctp: Allow version and upgrade channel to be specified in create #543

adamwg · 2024-05-29T17:12:00Z

Description of your changes

Add flags to specify the desired Crossplane version and upgrade channel in the
up ctp create command. Return an error if a version is specified with a
release channel other than None, since the server will accept a spec with a
version specified but ignore it in favor of the release channel's current
version.

For now, we don't have a programmatic way to fetch the valid release channels so
we hard-code the options. Similarly, we don't have an API to fetch supported
versions so we can't list them for the user in the CLI.

Fixes #541

I have:

Read and followed Upbound's contribution process.
Run make reviewable to ensure this PR is ready for review.
Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Manual testing of the following cases:

up ctp create without the new arguments works as before.
up ctp create --version=1.14.8-up.1 --channel=None is successful and the ctp has the right version.
up ctp create --version=1.14.8-up.1 throws an error since the default release channel is Stable.
up ctp create --channel=Rapid is successful and the ctp has the right release channel.

adamwg · 2024-05-29T17:17:40Z

cmd/up/controlplane/create.go

+	Crossplane struct {
+		Version     string `default:"" help:"The version of Universal Crossplane to use. The default depends on the selected auto-upgrade channel."`
+		AutoUpgrade struct {
+			Channel string `default:"Stable" help:"The auto-upgrade channel to use. Must be one of: None, Patch, Stable, Rapid" enum:"None,Patch,Stable,Rapid"`
+		} `embed:""`
+	} `embed:""`


Structured this to match the control plane API, so that it's easier to add additional options in the future without this struct becoming a huge, flat mess. Open to suggestions on the structure and naming of the flags - felt like --crossplane-version and --crossplane-autoupgrade-channel would be excessive, but I may have gone too far the other direction (--channel is a little cryptic?).

I like the code structure 👍🏼

channel is not a property of the control plane, but of the crossplane instance installed into it. So my suggestion would be --crossplane-channel and --crossplane-version. Wdyt?

I've been thinking of "a control plane" as being "a Crossplane", but from a product point of view I think that's not quite true (from a user point of view, there's a bit more to a control plane than just Crossplane). Adding the crossplane- prefix makes sense given that view of the world - updated to add it.

RedbackThomson · 2024-05-29T17:21:55Z

cmd/up/controlplane/create.go

+	Crossplane struct {
+		Version     string `default:"" help:"The version of Universal Crossplane to use. The default depends on the selected auto-upgrade channel."`
+		AutoUpgrade struct {
+			Channel string `default:"Stable" help:"The auto-upgrade channel to use. Must be one of: None, Patch, Stable, Rapid" enum:"None,Patch,Stable,Rapid"`
+		} `embed:""`
+	} `embed:""`


I like the code structure 👍🏼

channel is not a property of the control plane, but of the crossplane instance installed into it. So my suggestion would be --crossplane-channel and --crossplane-version. Wdyt?

RedbackThomson · 2024-05-29T17:22:06Z

cmd/up/controlplane/create.go

 	// todo(redbackthomson): Support all overrides for control planes
 	// ConfigurationName *string `help:"The optional name of the Configuration."`


We can probably delete this while we're at it

RedbackThomson · 2024-05-29T17:23:29Z

cmd/up/controlplane/create.go

+	if c.Crossplane.Version != "" && c.Crossplane.AutoUpgrade.Channel != string(spacesv1beta1.CrossplaneUpgradeNone) {
+		return fmt.Errorf("Upgrade channel must be %q to specify a version", string(spacesv1beta1.CrossplaneUpgradeNone))
+	}


Another validation that could happen here is checking to make sure the crossplane version does NOT have a leading v. vX.Y.Z is invalid - it should be just X.Y.Z. This is something we will eventually add to the server as well, but it is simple enough to add here. Otherwise trying to debug it, when applied, is a bit in the pain in the butt.

👍 Makes sense to me. I've added a validation that parses the provided version into a semver, which will catch some errors beyond just the leading v. Assuming that will be the most common error, though, I've clarified it in the error message since Invalid character(s) found on its own could be a little confusing for users:

% up ctp create --crossplane-version=v1.14.8-up.1 --crossplane-channel=None ctp-with-invalid-version up: error: controlplane (ctp) create: Invalid Crossplane version specified: Invalid character(s) found in major number "v1"; do not prefix the version with a 'v'

RedbackThomson · 2024-05-29T17:24:32Z

cmd/up/controlplane/create.go

@@ -59,6 +78,16 @@ func (c *createCmd) Run(ctx context.Context, p pterm.TextPrinter, upCtx *upbound
 		},
 	}

+	if c.Crossplane.Version != "" {
+		ctp.Spec.Crossplane.Version = &c.Crossplane.Version


Do you want to explicitly set the channel to none as well?

ctp.Spec.Crossplane.AutoUpgradeSpec = &spacesv1beta1.CrossplaneAutoUpgradeSpec{ Channel: ptr.To(spacesv1beta1.CrossplaneUpgradeChannelNone), }

As it works now, to specify a version the user must also provide --crossplane-channel=None. I kind of lean toward keeping it explicit like that, since the default channel is Stable and a user might expect to be able to specify a version while also keeping the Stable channel. Not super opinionated on this point, though - we could implicitly change the upgrade channel when a version is provided without a channel being specified.

RedbackThomson · 2024-05-30T21:44:10Z

cmd/up/controlplane/create.go

-
-	// todo(redbackthomson): Support all overrides for control planes
-	// ConfigurationName *string `help:"The optional name of the Configuration."`
+	} `embed:"" prefix:"crossplane-"`


This is such a cool feature I didn't know about. TIL

RedbackThomson · 2024-05-30T21:45:12Z

cmd/up/controlplane/create.go

-		return fmt.Errorf("Upgrade channel must be %q to specify a version", string(spacesv1beta1.CrossplaneUpgradeNone))
+	if c.Crossplane.Version != "" {
+		if c.Crossplane.AutoUpgrade.Channel != string(spacesv1beta1.CrossplaneUpgradeNone) {
+			return fmt.Errorf("Upgrade channel must be %q to specify a version", string(spacesv1beta1.CrossplaneUpgradeNone))


nit: All errors should begin with lowercase (the go error convention)

RedbackThomson · 2024-05-30T21:45:15Z

cmd/up/controlplane/create.go

+		}
+		_, err := semver.Parse(c.Crossplane.Version)
+		if err != nil {
+			return fmt.Errorf("Invalid Crossplane version specified: %w; do not prefix the version with a 'v'", err)


Add flags to specify the desired Crossplane version and upgrade channel in the `up ctp create` command. Return an error if a version is specified with a release channel other than `None`, since the server will accept a spec with a version specified but ignore it in favor of the release channel's current version. For now, we don't have a programmatic way to fetch the valid release channels so we hard-code the options. Similarly, we don't have an API to fetch supported versions so we can't list them for the user in the CLI. Fixes #541 Signed-off-by: Adam Wolfe Gordon <awg@upbound.io>

adamwg force-pushed the awg/ctp-create-version branch from 21c3e81 to 198dd99 Compare May 29, 2024 17:17

adamwg commented May 29, 2024

View reviewed changes

RedbackThomson reviewed May 29, 2024

View reviewed changes

RedbackThomson approved these changes May 30, 2024

View reviewed changes

adamwg added 2 commits May 31, 2024 14:59

ctp: Remove stale TODO in ctp create

b50a512

adamwg force-pushed the awg/ctp-create-version branch from 09aab99 to b50a512 Compare May 31, 2024 21:01

adamwg merged commit 653b9c4 into main May 31, 2024
6 checks passed

adamwg deleted the awg/ctp-create-version branch May 31, 2024 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ctp: Allow version and upgrade channel to be specified in create #543

ctp: Allow version and upgrade channel to be specified in create #543

adamwg commented May 29, 2024

adamwg May 29, 2024

RedbackThomson May 29, 2024

adamwg May 29, 2024

RedbackThomson May 29, 2024

RedbackThomson May 29, 2024

RedbackThomson May 29, 2024

adamwg May 29, 2024

RedbackThomson May 29, 2024

adamwg May 29, 2024

RedbackThomson May 30, 2024

RedbackThomson May 30, 2024

RedbackThomson May 30, 2024

		// todo(redbackthomson): Support all overrides for control planes
		// ConfigurationName *string `help:"The optional name of the Configuration."`

ctp: Allow version and upgrade channel to be specified in create #543

ctp: Allow version and upgrade channel to be specified in create #543

Conversation

adamwg commented May 29, 2024

Description of your changes

How has this code been tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment