Add controlplane create and delete commands #14

hasheddan · 2021-04-22T18:07:58Z

Also adds a common cloud.Context that we populate and pass to subcommands so that it can be progressively modified as argument tree is traversed.

Fixes #13

Updates method to get default cloud profile to also return id of the profile. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Introduces an internal cloud package with common functions that are used across mutltiple subgroups. Also introduces a cloud context which can be received by any command in the cloud group. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Refactors cloud and login commands to build config at the cloud level and pass it to any subcommands. This reduces the number of parameters that commands can receieve and also allows for progressive building of the config if applicable. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

up cloud controlplane create allows a user to create a new hosted control plane on Upbound cloud. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

up cloud controlplane delete allows a user to delete a control plane on Upbound Cloud. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Adds the controlplane commands subgroup to cloud commands and constructs a common client that can be received by all controlplane subcommands. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Overrides control-plane to controlplane and also support alias to xp. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Changes up cloud controlplane alias to up cloud xp. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Adds comments to exported functions in the cloud package. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan · 2021-04-22T21:08:53Z

cmd/up/cloud/controlplane/delete.go

+	// TODO(hasheddan): consider using name instead of ID
+	ID uuid.UUID `arg:"" required:"" help:"ID of control plane."`


Note that we are currently using the control plane ID. We may opt to do this by name in the future, but I am sticking with this implementation for the time being pending implementing a new API endpoint for deleting by name. If we do not end up adding that endpoint, we would need to list control planes for the user and match id to provided name before calling delete.

Updates the extraction performed at the cloud config level to only fetch the config itself, but not attempt to identify the profile. This is more accommodating for allowing both commands that require the profile and those which create a profile (i.e. login) to consume this functionality. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

turkenh · 2021-04-26T20:19:10Z

cmd/up/cloud/controlplane.go

+		}
+		cloudCtx.ID = id
+	} else {
+		profile, err = cloudCtx.Cfg.GetCloudProfile(cloudCtx.ID)


It looks like we identify profiles per id (username, email or token ID). This sounds a bit limited/confusing to me, since profile also keeps org name as well. So, as hasan@upbound.io, I may want to have two profiles, one for my user org (or namespace) and one for hasan-testorg and this seems not to be possible. I thought we have ability to name profiles independently similar to aws cli: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-profiles.html

We do restrict the default org for a given ID, but you can always override for any command by passing -o myorg so I don't feel like this is super restrictive. To me it feels a bit more confusing how AWS does it where your profile name may be somewhat arbitrary. I like that we say "when you are authenticated with this user you will use this default account", but you can always override at command invocation time.

yeah, but there is no one-to-one mapping between a user and a profile (which includes org).
as mentioned, users might want to define multiple profiles for 1 user for different orgs. Of course, one can always override default org with the flag but this is a bit confusing UX-wise since it does not provide expected functionality especially since we support multiple profiles.

Maybe, I am expecting something different than the intended functionality of "profiles" but gcloud also allows giving arbitrary names to configurations: https://cloud.google.com/sdk/gcloud/reference/config/configurations/create

I would prefer to key profiles as something constant (for now?) like default and leave the possibility to extend with adding/removing/updating new (named) profiles through cli.

Hmm, I personally don't prefer this pattern as it requires passing something like -p my-profile rather than -u hasheddan, but I do agree that it is more flexible for future changes. That being said, we don't currently have as complex of an auth model as GCP and AWS (I was looking more at how Docker stores configuration as reference here). How would you feel about me following up with a revision to how we store credentials after this PR as it will deviate from the agreed upon design so we will likely need more folks to weigh in?

Yeah, that sounds good to me.
I think I am mostly confused since we seem to support multiple profiles but still want to keep things simple :)

turkenh · 2021-04-26T20:26:46Z

cmd/up/cloud/cloud.go

+		org = c.Organization
+	}
+	if org == "" {
+		org = id


This defaulting seems problematic:

If identifier is a token or email, this wouldn't work since no corresponding org (uuid or email)

Org, I set in my profile will always be overridden?

Good catch -- the idea here was that we would do this defaulting at cloud level, then override in subgroups (like controlplane) when we actually extract the profile. However, this needs to be revised to:

At cloud level we will set cloudCtx.Org to c.Organization.

In controlplane we will extract profile and if cloudCtx.Org == "", we will set it to the default in the profile.

If there is no default org in the profile, we try with the id (which will fail if not username).

@turkenh updated here 👍

turkenh · 2021-04-26T20:29:42Z

cmd/up/cloud/controlplane/create.go

+// Run executes the create command.
+func (c *CreateCmd) Run(kong *kong.Context, client *cp.Client, cloudCtx *cloud.Context) error {
+	_, err := client.Create(context.Background(), &cp.ControlPlaneCreateParameters{
+		Namespace:   cloudCtx.Org,


Did we use org in profile anywhere?

It should be extracted at controlplane level, but that was missed so will update 👍

turkenh · 2021-04-26T20:31:02Z

cmd/up/cloud/controlplane/create.go

+	_, err := client.Create(context.Background(), &cp.ControlPlaneCreateParameters{
+		Namespace:   cloudCtx.Org,
+		Name:        c.Name,
+		Description: c.Description,


Any reason not to support passing self-hosted flag as well?

self-hosted will be encompassed by up cloud controlplane attach (see #4) because there was a desire to keep the hosted and self-hosted flows completely separate

turkenh · 2021-04-26T20:39:06Z

internal/cloud/context.go

+	Cfg      *config.Config
+	CfgSrc   config.Source


Just wondering whether it would be cleaner if we could handle config/profile handling/defaulting before passing context down to individual commands and only have Session here instead of Cfg and CfgSrc.

I actually did that initially, but that doesn't work for commands like up cloud login which need access to actually write / modify the config file. This will also be true for user and org commands that update defaults

Updates cloud hook to not default org to id and instead defer to after profile extraction so that the default profile org will be used when set. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

turkenh

Thanks, @hasheddan, lgtm!
Although I have some comments around UX, I agree that this could be improved with follow-up PRs/discussions and should not be a blocker here.

turkenh · 2021-04-26T22:41:36Z

cmd/up/cloud/controlplane.go

+	}
+	// If no org is set in profile, use the ID.
+	if cloudCtx.Org == "" {
+		cloudCtx.Org = cloudCtx.ID


I would prefer to fail explicitly (e.g. no org set) here if id is not a username. Otherwise, the error would be something like, no org named hasan@upbound.io or no org named 11501427-76ee-4da8-9147-4bf92abfe4be which might be hard to figure out.

@turkenh that's fair 👍 That being said, I would like to default to the user account if no org is specified. We could move this to functionality to login though so that we actually set the default org in the config.json if logging in as user, then error if no default is set. I would like to also follow up with this in the potential changes to the profile because it will be affected by that behavior. I am going to be writing up an issue today for folks to weigh in on :)

turkenh · 2021-04-26T22:46:18Z

cmd/up/cloud/controlplane.go

+		}
+		cloudCtx.ID = id
+	} else {
+		profile, err = cloudCtx.Cfg.GetCloudProfile(cloudCtx.ID)


Yeah, that sounds good to me.
I think I am mostly confused since we seem to support multiple profiles but still want to keep things simple :)

* fix(xpkg): func xpkg dep for functions Signed-off-by: Christopher Haar <christopher.haar@upbound.io> * fix(xpkg): func xpkg dep for functions Signed-off-by: Christopher Haar <christopher.haar@upbound.io> * feat(bump): update native list because of new resource-kinds after bump k8 versions Signed-off-by: Christopher Haar <christopher.haar@upbound.io> * feat(xpkg): dep tests adoption Signed-off-by: Christopher Haar <christopher.haar@upbound.io> * fix(xpkg): func xpkg dep for functions Signed-off-by: Christopher Haar <christopher.haar@upbound.io> --------- Signed-off-by: Christopher Haar <christopher.haar@upbound.io>

hasheddan mentioned this pull request Apr 22, 2021

Temporarily enable ability to consume private modules #15

Merged

hasheddan added 8 commits April 22, 2021 15:46

Return id when fetching default cloud profile

f10bc61

Updates method to get default cloud profile to also return id of the profile. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Add controlplane create command

17765a5

up cloud controlplane create allows a user to create a new hosted control plane on Upbound cloud. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Add controlplane delete command

d6b0ce7

up cloud controlplane delete allows a user to delete a control plane on Upbound Cloud. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Add controlplane subgroup to cloud commands

170f1fb

Adds the controlplane commands subgroup to cloud commands and constructs a common client that can be received by all controlplane subcommands. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Override default controlplane command naming and add alias

c0cf58a

Overrides control-plane to controlplane and also support alias to xp. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

Update command docs to reflect accurate xp alias

671d5ac

Changes up cloud controlplane alias to up cloud xp. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan force-pushed the impl-create-cp branch from c5db07e to 671d5ac Compare April 22, 2021 20:46

Add comment to exported helpers in cloud package

fcc67cc

Adds comments to exported functions in the cloud package. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan requested review from AaronME, muvaf, turkenh and ulucinar April 22, 2021 21:00

hasheddan commented Apr 22, 2021

View reviewed changes

hasheddan force-pushed the impl-create-cp branch from 859704c to 2d883d5 Compare April 23, 2021 20:26

turkenh reviewed Apr 26, 2021

View reviewed changes

Do not default org until profile extraction

4527da2

Updates cloud hook to not default org to id and instead defer to after profile extraction so that the default profile org will be used when set. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>

hasheddan requested a review from turkenh April 26, 2021 21:47

turkenh approved these changes Apr 26, 2021

View reviewed changes

hasheddan merged commit be2c26a into upbound:main Apr 27, 2021

hasheddan mentioned this pull request Apr 28, 2021

Formalize structure of cloud profiles (for initial release) #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add controlplane create and delete commands #14

Add controlplane create and delete commands #14

hasheddan commented Apr 22, 2021

hasheddan Apr 22, 2021

turkenh Apr 26, 2021

hasheddan Apr 26, 2021

turkenh Apr 26, 2021

turkenh Apr 26, 2021 •

edited

Loading

hasheddan Apr 26, 2021

turkenh Apr 26, 2021

turkenh Apr 26, 2021

hasheddan Apr 26, 2021

hasheddan Apr 26, 2021

turkenh Apr 26, 2021

hasheddan Apr 26, 2021

turkenh Apr 26, 2021

hasheddan Apr 26, 2021

turkenh Apr 26, 2021

hasheddan Apr 26, 2021

turkenh left a comment

turkenh Apr 26, 2021 •

edited

Loading

hasheddan Apr 27, 2021

turkenh Apr 26, 2021

		// TODO(hasheddan): consider using name instead of ID
		ID uuid.UUID `arg:"" required:"" help:"ID of control plane."`

Add controlplane create and delete commands #14

Add controlplane create and delete commands #14

Conversation

hasheddan commented Apr 22, 2021

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

turkenh Apr 26, 2021 • edited Loading

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

turkenh left a comment

Choose a reason for hiding this comment

turkenh Apr 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turkenh Apr 26, 2021 •

edited

Loading

turkenh Apr 26, 2021 •

edited

Loading