Allow creating new space profiles by name #400

branden · 2023-10-26T03:39:55Z

Description of your changes

This makes up profile set space accept a nonexistent --profile. It will create that profile.

Fixes #399.

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

Added unit tests.

branden@crateria kind % up profile list
CURRENT   NAME      TYPE    ACCOUNT   KUBECONFIG   KUBECONTEXT
*         default   space                          kind-kind

branden@crateria kind % up profile set space --profile=new
Profile "new" updated to use context "kind-kind" from the default kubeconfig and selected as the default profile

branden@crateria kind % up profile list
CURRENT   NAME      TYPE    ACCOUNT   KUBECONFIG   KUBECONTEXT
          default   space                          kind-kind
*         new       space                          kind-kind

branden@crateria kind % up profile set space --profile=default
Profile "default" updated to use context "kind-kind" from the default kubeconfig and selected as the default profile

branden@crateria kind % up profile list
CURRENT   NAME      TYPE    ACCOUNT   KUBECONFIG   KUBECONTEXT
*         default   space                          kind-kind
          new       space                          kind-kind

branden@crateria kind %

sttts · 2023-10-26T12:42:08Z

cmd/up/profile/profile.go

@@ -37,7 +37,7 @@ type Cmd struct {
 // AfterApply constructs and binds Upbound-specific context to any subcommands
 // that have Run() methods that receive it.
 func (c *Cmd) AfterApply(kongCtx *kong.Context) error {
-	upCtx, err := upbound.NewFromFlags(c.Flags)
+	upCtx, err := upbound.NewFromFlags(c.Flags, upbound.AllowMissingProfile())


please fix the command description 10 lines above. This is not what the command is doing.

And be very explicit that this overrides the current profile.

sttts · 2023-10-26T12:43:13Z

cmd/up/profile/set.go

@@ -44,13 +44,10 @@ func (c *spaceCmd) AfterApply(kongCtx *kong.Context) error {
 }


Please implement a proper one or two paragraph help text.

sttts · 2023-10-26T12:44:27Z

We should also choose a new profile name in space init. Either here or in another PR.

sttts · 2023-10-26T12:45:43Z

cmd/up/profile/set.go

-		p.Print(" and selected as the default profile")
-	}
+	p.Printf(
+		"Profile %q updated to use context %q from the %s and selected as the default profile",


"context" is not a term. Maybe "talk to Spaces via the current Kubernetes context %q" ?

Do we check that it actually is a Spaces installation? We should.

sttts · 2023-10-26T12:46:57Z

cmd/up/profile/set_test.go

+	return nil
+}
+
+func TestSpaceRun(t *testing.T) {


sttts · 2023-10-26T12:48:52Z

cmd/up/profile/set.go

-			return errors.Wrap(err, errSetProfile)
-		}
+	if err := upCtx.Cfg.SetDefaultUpboundProfile(upCtx.ProfileName); err != nil {
+		return errors.Wrap(err, errSetProfile)


technically selecting a profile is more than a set command should do. We have profile use for that purpose.

No strong opinion here. Just be very clear in the command documentation that profile selection happens.

sttts · 2023-10-26T12:50:06Z

cmd/up/profile/set_test.go

+				Kube: upbound.KubeFlags{
+					Kubeconfig: kubeconfig,
+				},
+			},


nit: isn't the cmd always the same? Why not create it below in the loop?

branden · 2023-10-26T17:12:37Z

@sttts

We should also choose a new profile name in space init. Either here or in another PR.

Do we check that it actually is a Spaces installation? We should.

I'd like to keep this PR scoped to addressing #399. Could you file issues for these so we don't lose track of them?

branden · 2023-10-26T18:26:28Z

@sttts I pushed some changes to address your comments, lmk what you think. Notably I put things back so that profile set space only updates the default profile when there are no existing profiles, since like you pointed out we already have profile use.

sttts · 2023-10-26T18:54:43Z

Created #404 for the space init part.

sttts · 2023-10-26T18:56:36Z

cmd/up/profile/profile.go

@@ -29,15 +29,15 @@ type Cmd struct {
 	Use     useCmd     `cmd:"" help:"Set the default Upbound Profile to the given Profile."`
 	View    viewCmd    `cmd:"" help:"View the Upbound Profile settings across profiles."`
 	Config  config.Cmd `cmd:"" help:"Interact with the current Upbound Profile's config."`
-	Set     setCmd     `cmd:"" help:"Create an Upbound Profile."`
+	Set     setCmd     `cmd:"" help:"Create or update an Upbound Profile."`


Set a profile (the current by default) to talk to a Space? set alone is not usable today. I would be explicit here, until we have another set subcommand.

sttts · 2023-10-26T18:59:38Z

cmd/up/profile/space_help.txt

+A Space profile communicates with a Space using a Kubernetes context from a kubeconfig
+file. The kubeconfig and context may be selected using the --kubeconfig and
+--kubecontext flags. If --kubeconfig is not provided, then the profile will use
+the default kubeconfig at ~/.kube/config. If --kubecontext is not provided, then
+the profile will use the kubeconfig's current default context.


Suggested change

A Space profile communicates with a Space using a Kubernetes context from a kubeconfig

file. The kubeconfig and context may be selected using the --kubeconfig and

--kubecontext flags. If --kubeconfig is not provided, then the profile will use

the default kubeconfig at ~/.kube/config. If --kubecontext is not provided, then

the profile will use the kubeconfig's current default context.

Set a profile to be a Space profile. Use --profile to create a new profile.

By default the current profile is overwritten.

A Space profile communicates with a Space using a Kubernetes context from a kubeconfig

file. The kubeconfig and context may be selected using the --kubeconfig and

--kubecontext flags. If --kubeconfig is not provided, then the profile will use

the default kubeconfig at ~/.kube/config. If --kubecontext is not provided, then

the profile will use the kubeconfig's current default context.

Haven't looked deeply. The context name is stored, right? So the context selection happens on set space, not when a Space profile is used. This is not very clear in the current text.

Yeah that's correct. Good point, I'll clarify this.

branden · 2023-10-26T19:19:45Z

@sttts Here's how help text looks after the latest push:

branden@crateria up % ./_output/bin/darwin_arm64/up profile set -h
Usage: up profile set <command>

Set an Upbound Profile for use with a Space.

Flags:
  -h, --help                Show context-sensitive help.
      --format="default"    Format for get/list commands. Can be: json, yaml,
                            default
  -v, --version             Print version and exit.
  -q, --quiet               Suppress all output.
      --pretty              Pretty print output.

      --domain=https://upbound.io
                            Root Upbound domain ($UP_DOMAIN).
      --profile=STRING      Profile used to execute command ($UP_PROFILE).
  -a, --account=STRING      Account used to execute command ($UP_ACCOUNT).
      --insecure-skip-tls-verify
                            [INSECURE] Skip verifying TLS certificates
                            ($UP_INSECURE_SKIP_TLS_VERIFY).
  -d, --debug=INT           [INSECURE] Run with debug logging. Repeat
                            to increase verbosity. Output might contain
                            confidential data like tokens ($UP_DEBUG).

Commands:
  profile set space    Set an Upbound Profile for use with a Space.
branden@crateria up % ./_output/bin/darwin_arm64/up profile set space -h
Usage: up profile set space

Set an Upbound Profile for use with a Space.

By default the default profile is overwritten. Use --profile to create or update
a different profile.

A Space profile communicates with a Space using a Kubernetes context from
a kubeconfig file. The kubeconfig and context may be selected using the
--kubeconfig and --kubecontext flags. If --kubeconfig is not provided, then the
profile will use the default kubeconfig at ~/.kube/config. If --kubecontext is
not provided, then the profile will use the kubeconfig's current default context
at the time this command is run. Changing the kubeconfig's default context
afterward will not change which context the profile uses.

Flags:
  -h, --help                  Show context-sensitive help.
      --format="default"      Format for get/list commands. Can be: json, yaml,
                              default
  -v, --version               Print version and exit.
  -q, --quiet                 Suppress all output.
      --pretty                Pretty print output.

      --domain=https://upbound.io
                              Root Upbound domain ($UP_DOMAIN).
      --profile=STRING        Profile used to execute command ($UP_PROFILE).
  -a, --account=STRING        Account used to execute command ($UP_ACCOUNT).
      --insecure-skip-tls-verify
                              [INSECURE] Skip verifying TLS certificates
                              ($UP_INSECURE_SKIP_TLS_VERIFY).
  -d, --debug=INT             [INSECURE] Run with debug logging. Repeat
                              to increase verbosity. Output might contain
                              confidential data like tokens ($UP_DEBUG).

      --kubeconfig=STRING     Override default kubeconfig path.
      --kubecontext=STRING    Override default kubeconfig context.
branden@crateria up %

sttts · 2023-10-26T19:50:02Z

@branden perfect help text. Thanks!

sttts · 2023-10-26T19:51:44Z

Only quesiton that is left is about checking that it is a Space kube context. Checking for e.g. the mxe-controller deployment in upbound-system. Would do.

Then we are ready to go.

sttts · 2023-10-31T07:00:07Z

The Spaces check is a little beyond scope technically. Approving.

branden added 2 commits October 25, 2023 20:20

Allow creating new space profiles by name

25acdd2

Always select new or updated profile as the default

0fcc70d

branden requested a review from a team October 26, 2023 03:39

Add tests for profile set space

58a9018

branden force-pushed the set-create-profile branch from 9b1e730 to 58a9018 Compare October 26, 2023 04:10

sttts reviewed Oct 26, 2023

View reviewed changes

branden added 4 commits October 26, 2023 11:06

Only update default profile if none exists

d76fd25

Fixup spaceCmd.Run() test

f802d60

Clarify that this is a Kubernetes context in output

2c9adef

Improve profile set help text

d1c488d

branden requested a review from sttts October 26, 2023 18:26

Add verbose help for profile set space

a9e02ef

sttts reviewed Oct 26, 2023

View reviewed changes

Improve help text

53f5d84

sttts approved these changes Oct 31, 2023

View reviewed changes

sttts merged commit cc61c8c into upbound:main Oct 31, 2023
6 checks passed

sttts mentioned this pull request Oct 31, 2023

up/profile: check for Spaces on 'up profile set space' #406

Merged

3 tasks

branden deleted the set-create-profile branch October 31, 2023 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow creating new space profiles by name #400

Allow creating new space profiles by name #400

branden commented Oct 26, 2023 •

edited

Loading

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts commented Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023

branden commented Oct 26, 2023

branden commented Oct 26, 2023

sttts commented Oct 26, 2023

sttts Oct 26, 2023

sttts Oct 26, 2023 •

edited

Loading

sttts Oct 26, 2023

branden Oct 26, 2023

branden commented Oct 26, 2023

sttts commented Oct 26, 2023

sttts commented Oct 26, 2023

sttts commented Oct 31, 2023

		@@ -44,13 +44,10 @@ func (c spaceCmd) AfterApply(kongCtx kong.Context) error {
		}

Allow creating new space profiles by name #400

Allow creating new space profiles by name #400

Conversation

branden commented Oct 26, 2023 • edited Loading

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

sttts commented Oct 26, 2023

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

branden commented Oct 26, 2023

branden commented Oct 26, 2023

sttts commented Oct 26, 2023

Choose a reason for hiding this comment

sttts Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

branden commented Oct 26, 2023

sttts commented Oct 26, 2023

sttts commented Oct 26, 2023

sttts commented Oct 31, 2023

branden commented Oct 26, 2023 •

edited

Loading

sttts Oct 26, 2023 •

edited

Loading