-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add support for up ctp provider install
and up ctp configuration install
#261
Conversation
Moves the dynamic watch logic to the kube package such that it can be used across commands. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Creates an upterm package that contains pterm styling used across commands. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Updates upbound subcommands to consume functionality from the shared dynamic watcher and pterm styles package. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds a package resource to the resources package to make accessing common conditions simpler. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds a pkg command group, which currently only includes an install command, but can be used for both providers and configurations when included by parent command. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds configuration and provider install commands to the ctp group. The same struct is used for both commands, but variable interpolation is used to parameterize based on the subtree from which the command is invoked. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Adds support for --package-pull-secrets to allow specifying pull secrets on package installation. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
// NOTE(hasheddan): kong automatically cleans paths tagged with existingfile. | ||
Kubeconfig string `type:"existingfile" help:"Override default kubeconfig path."` | ||
Name string `help:"Name of ${package_type}."` | ||
PackagePullSecrets []string `help:"List of secrets used to pull ${package_type}."` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a user-facing help string? If I was the user, I'd ask if the list is space-delimited, comma-delimited, or something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlainRoy kong
mostly handles this for us by displaying comma-separated, but open to feedback on the formatting (we can override pretty much everything):
--package-pull-secrets=PACKAGE-PULL-SECRETS,... List of secrets used to pull Provider
If we do want to override this, we should do it across the board, which I'll likely break into a separate chunk of work ππ»
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that it shows an example with a comma, I'm happy. I might be tempted to change "List" to "Comma-separated list", but I really don't feel strongly since the example makes it clear.
internal/kube/watch.go
Outdated
@@ -0,0 +1,76 @@ | |||
// Copyright 2021 Upbound Inc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2021?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! These are auto-generated from a template, time to bump this :)
Updates header template to 2022. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @hasheddan! Just some non-blocking comments and a nit. Feel free to act on those if you wish π.
Configuration pkg.Cmd `cmd:"" set:"package_type=Configuration" help:"Manage Configurations."` | ||
Provider pkg.Cmd `cmd:"" set:"package_type=Provider" help:"Manage Providers."` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking]: Is this the first place we using multi-word variables in the Kong declarations? The snake case stands out to me. It looks like the other place we have a multi-word variable is controlplane
which is all lowercase. I'm assuming snakecase is used for legibility (π ), curious if there was a active choice to go that path over camelcase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnthornton this actually is just internal machinery for variable interpolation, it is never surfaced to the end user ππ»
) | ||
|
||
type Package struct { | ||
unstructured.Unstructured |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not the first time we've used this pattern (across repos) π. Before too long it might be worth considering moving these resource operations to a shared place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree on that ππ» Thought about doing a larger refactor here, but decided to keep it for now until we feel a bit more pain around it
internal/upterm/styles.go
Outdated
RaisedPrefix = pterm.Prefix{ | ||
Style: &pterm.Style{pterm.FgLightMagenta}, | ||
Text: " π", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - thoughts on keeping the prefixes together? i.e. this was separated from the EyesPrefix and it's not clear why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally makes sense, didn't realize I had separated them ππ»
Organizes printer prefixes to live next to each other. Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Description of your changes
Adds configuration and provider install commands to the ctp group. The
same struct is used for both commands, but variable interpolation is
used to parameterize based on the subtree from which the command is
invoked.
Signed-off-by: hasheddan georgedanielmangum@gmail.com
Fixes #159
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
Verified successful install of providers and configurations, as well as updates to functionality in
up upbound
commands.