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

CLI for kapp-controller #412

Closed
52 of 62 tasks
renuy opened this issue Oct 26, 2021 · 22 comments
Closed
52 of 62 tasks

CLI for kapp-controller #412

renuy opened this issue Oct 26, 2021 · 22 comments
Assignees
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request

Comments

@renuy
Copy link

renuy commented Oct 26, 2021

Describe the problem/challenge you have
There is no interface for users to interact with kapp-controller server-side.

Describe the solution you'd like
a CLI which can be used to interact with kapp-controller

Milestone 1 -- Parity

  • package available get
  • package available list
  • package install
    • package installed create (alias to "package install")
    • check values-file existence before trying to do things
  • package installed get
  • package installed list
  • package installed delete
  • package installed update
    • check values-file existence before trying to do things
  • package repository get
  • package repository list
  • package repository create
  • package repository delete
  • package repository update
  • Clean up (In Progress)
    • openapi v3 parsing (types)
  • Tests (In Progress)
  • Update root command name? (kapp -> kctrl???)
  • values in/out clean up (single values only supported)
  • remove wait for delete of package install
  • idempotency of install (already in downstream: make package install idempotent vmware-tanzu/tanzu-framework#983)
  • PR to develop
  • documentation PR listing out commands and workflows (Ongoing and updated PR)

Milestone 1 -- Feedback

  • Error out when --version is not specified by user while installing package
  • Remove --service-account-name flag from package installed update
  • Ensure that if a values secret exists with a key other than valuesFileKey, update existing key
    • Blow up if more than one keys already exist
  • Update only Fetch field instead of whole Spec when updating a package repository.
  • Update reconciliation messages to include resource description
  • Remove usage of openapi3 library?
  • Remove create namespace flag.
  • openapi schema fields should be sorted alpha
  • add github action test workflow
  • Add examples for commands
  • ensure that progress waiting output is not repeated (was prev problem with spinner impl)

Milestone 2 -- Consuming as library

  • importable command tree
  • UI Presentation
  • argument passing
  • global flags
  • Handle legacy Tanzu annotations and carvel annotations better
  • compile a list of differences from current tanzu package and kctrl (breaking changes)
  • sorting semvers via semver library
  • Support stdin input (e.g. -) for values file

Milestone 3 -- Enhancements

  • get, list, delete commands for app
  • status, kick and pause commands for app
  • tail progress of app installation includes easily grabbing failure information from app
  • tail progress of pkgi/pkgr installation using underlying apps
  • tail progress in kick and delete for app
  • kick, pause and status commands for pkg
  • tail progress for kick & delete commands for pkg

Milestone 4 -- Enhancements


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@renuy renuy added enhancement This issue is a feature request carvel-triage This issue has not yet been reviewed for validity labels Oct 26, 2021
@renuy renuy added carvel-accepted This issue should be considered for future work and that the triage process has been completed and removed carvel-triage This issue has not yet been reviewed for validity labels Oct 28, 2021
@100mik
Copy link
Contributor

100mik commented Oct 28, 2021

Food for thought:

  • How can we best handle resources created (ServiceAccounts, Secrets, ClusterRoles, ClusterRoleBindings) and track them for deletion effectively. (Currently, we plan on using annotations on PackageInstall resources and the resources themselves)
  • Do users wanna dump values fetched from Secret references in a PackageInstall?

@100mik
Copy link
Contributor

100mik commented Nov 18, 2021

@cppforlife vmware-tanzu/tanzu-framework#983 seems to add fallback behaviour for all creates and updates, and improve logs printed while removal and creation of created resources. We have already incorporated these changes

@jorgemoralespou
Copy link

Could this CLI be also compatible somehow to be a kubectl plugin?

@100mik
Copy link
Contributor

100mik commented Nov 30, 2021

We do have a method which attaches the package command tree to a Cobra cmd, since we would like to see the package commands consumed by the package plugin for tanzu-framework

(We might have to take a harder look at it if it is used in a kubectl plugin as I believe a bunch of flags might have overlapping names, not sure how that works out)

@jorgemoralespou
Copy link

Since kapp-controller CLI is targetting kubernetes users, I would really suggest that it's implemented as a kubectl plugin to boost adoption and minimize learning curve. If at the end of the day, this is basically providing the same functionality as the tanzu package plugin, it might then be better to just work on making that one standalone as well as tanzu plugin and call it a day.

@100mik
Copy link
Contributor

100mik commented Nov 30, 2021

We do intend to add a set of app commands which are intended to improve how our users observe and debug App CRs.

@danielhelfand
Copy link
Contributor

Hopefully it is the case that this cli can be applied to both contexts. I think a kubectl plugin would be really worthwhile, and I tend to agree that I was thinking this cli should be more tailored to open source users.

Here's more on creating plugins for kuebctl: https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins/

@joe-kimmel-vmw
Copy link
Contributor

would it make sense to have a "--force" delete feature that was more powerful than kubectl's? e.g. see this issue: #416

so i could imagine that this CLI (or kapp itself) could be a place where we add like a --nuke-it-from-space flag that:

  • removes all finalizers from the CR specified and tries to delete it
  • if it's still there sets: spec.noopDelete=true on the CR and deletes it again

Just because I can imagine how we could , doesn't make it a good idea (similar i think to a theme park filled with dinosaurs?), so i'm asking whether we should. :)

@jorgemoralespou
Copy link

I wouldn't consider introducing a flag to undo things that should not have happened, just for the sake of convenience, a good UX.

@danielhelfand
Copy link
Contributor

Some minor UX observations for kctrl package available get:

Why does kctrl package available get require the -p flag? While trying to use, I couldn't find any examples of using it with the help command. From my own expectations, I was thinking the command was as follows:

$ kctrl package available get cert-manager.community.tanzu.vmware.com

It seems like the flag is not needed, but maybe I am not understanding other design considerations.

Another interesting observation I had with kctrl package available get was that there was no message saying no package found for nonexistent packages:

$ kctrl package available get -p i-dont-exist                       
Target cluster 'https://192.168.64.225:8443' (nodes: minikube)

Name  i-dont-exist  

Version  Released at  

Succeeded

It might be nice to do what kubectl does in this scenario which is saying no resources found:

kubectl get deployments/i-dont-exist         
Error from server (NotFound): deployments.apps "i-dont-exist" not found

@100mik
Copy link
Contributor

100mik commented Dec 12, 2021

@joe-kimmel-vmw That does seem like a recurring issue we have to assist users with often if I am not wrong, it definitely makes sense to have something like that albeit with a less "aggressive" name maybe? 🤔

@100mik
Copy link
Contributor

100mik commented Dec 12, 2021

@danielhelfand thanks for pointing that out! we should definitely be saying that we could not find any resources if that is the case while getting a specific resource.

Regarding how we are passing parameters we made the choice of using flags for "passing values" to the CLI rather than having positional arguments as that is a observed pattern across the other Carvel CLIs as well which we wanted to maintain.
Adding examples for the commands is on our radar! I will go ahead and add it to our TODO list!

@100mik
Copy link
Contributor

100mik commented Dec 16, 2021

@jorgemoralespou pointed out that we should allow the users to update a package install to stop referencing a secret. Good catch!

We can update a PackageInstall to stop referencing a Secret and delete the Secret if it was created by the CLI.

As of today if users would want to update the installation to not consume a supplied values file, a workaround is to supply an empty values file. In which case the CLI updates the secret to have an empty values file.
We could let the users have a less hack way of doing this.

Option 1

In case the user does not supply a value file, or supplies an empty value file while updating an installation. We could prompt the user to confirm whether they would like to the Secret to dereferenced and deleted (if created by the CLI)

This might not be the best way to go about it as this case will be observed mainly when users are trying to bump the version of their installation.

Option 2

Another option would be to have a flag like --delete-unused-secret or --dereference-unused-secret which the user can use in case they want the Secret to be garbage collected if an empty values file or no values file is supplied while updating the installation.
This would probably be the preferred option.

Adding this to the list of AIs for now, let's see how we can approach this.

@praveenrewar
Copy link
Member

I would prefer --delete-secret instead of --delete-unused-secret as unused could be a bit confusing.

@100mik
Copy link
Contributor

100mik commented Dec 16, 2021

That was my first pref too, I was wondering if somehow the values file being supplied is dynamic, would users want to keep the flag in case they wanna dereference a secret only if the values file supplied is empty. But I guess that is a bit of a stretch.

@jorgemoralespou
Copy link

I would rather say --no-values-file as that's the flag you use when you create the secret. In fact, for the user, the secret is an implementation detail that she might not need to know. She created a package with values-file, and now she decides no values-file is needed. We should take the rest (deleting the secret, etc...) internally.

@100mik
Copy link
Contributor

100mik commented Dec 16, 2021

--no-values-file does make sense. I think abstracting the inner workings will benefit users

@stealthybox
Copy link

On the carvel community call today, we talked about how kick has advantages over reconcile in that it's shorter to say, less abstract, and also fun.
However it does feel a little bit physical which could mean it's not an optimal word choice.

Then we started talking about how nudge feels much nicer.
It's nice and short, a single syllable, and really captures the intent of what you would do if somebody needs a gentle reminder to prioritize something.

For the help text, we would probably also still include the word reconcile to explain/convey what happens.

@jorgemoralespou
Copy link

Then we started talking about how nudge feels much nicer

You should probably think about non-native english speakers. I don't even know what nudge means. So, I would definitely not know what it does.

@stealthybox
Copy link

For sure -- kick is maybe a more common word?
I would think that reconcile is more obscure than nudge, but contextually in the cloud-native space non-native speakers are going to quickly learn what that means.

@100mik
Copy link
Contributor

100mik commented May 10, 2022

Closing this off in favour of issues branched out from it.
Thanks to all the awesome folks who engaged with us in this thread ❤️
Feel free to raise further concerns, issues or enhancement requests by filing an issue 🚀
(Adding the cli label will us manage them better!)

@100mik 100mik closed this as completed May 10, 2022
@100mik
Copy link
Contributor

100mik commented May 10, 2022

PS: Added links to the issues that have branched off in the description of this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel-accepted This issue should be considered for future work and that the triage process has been completed enhancement This issue is a feature request
Projects
None yet
Development

No branches or pull requests

7 participants