Create and Duplicate Resource actions#2563
Conversation
2e3a5d2 to
4da73bb
Compare
|
Fixed the failing test. I forgot to make use of the mock lookPath implementation for testing... 🙈 |
116328e to
0a72f43
Compare
|
Hi @nobbs |
This commit refactors the code from PR derailed#954 to make it work with current master, as well as only enable the "Create" and "Duplicate" actions when K9s is not in read-only mode. Also, both actions will use `kubectl create` instead of `kubectl apply` to create the resource, as this will automatically prevent the user from overwriting an existing resource by name.
0a72f43 to
0ba3bd2
Compare
derailed
left a comment
There was a problem hiding this comment.
@nobbs First off Alexej thank you for this update and your attention to details!
I have to say I am struggling with this a bit. I can see the usefulness while experimenting with a cluster. But as I've voiced before I am not keen on cluster cowboying.
Moreover users would likely leverage helm/kustomize/... to provision their clusters. Hence the one offs could be duds.
Imho I think the create functionality is a dud. Don't think there is value starting from a blank slate. Duplicating a res may make sense but the amount of editing and corrections required could also be daunting ie users would have to plow thru managed fields + additional instance specific bits injected by the control plane ie uuid, timestamps etc...
Also k9s does have a dir view which could be a better place to land this as this is where the cluster manifests actually reside. Hence we would not have to hack screen dumps to persist to disk.
I think for this feature to actually make sense would be to actually leverage the context at hand. For example if the user is in pod or cm view a pod/cm artifact skeleton with some filled in fields ie name/namespace or others based on current context.
Would this make sense?
| func (b *Browser) createCmd(_ *tcell.EventKey) *tcell.EventKey { | ||
| tmpFile, err := createTmpYaml() | ||
| if err != nil { | ||
| b.App().Flash().Err(errors.New("Failed to create temporary resource file: " + err.Error())) |
There was a problem hiding this comment.
Errors in go should not be capitalized. Also prefer wrapping the error ie fmt.Errorf("failed ...: %w", err)
Please follow similar logic for here down.
|
|
||
| content, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| app.Flash().Errf("Failed to create resource from file %s", err) |
There was a problem hiding this comment.
We are reading the file at this stage. The error should reflect that
| func isFileEmpty(filePath string) (bool, error) { | ||
| fileStat, err := os.Stat(filePath) | ||
| if err != nil { | ||
| return false, errors.New("Failed to get temporary resource file information: " + err.Error()) |
There was a problem hiding this comment.
Prefer error wrapping as mentioned above
@derailed: Not sure if it will help, but I thought it might be useful to convey my experience here: When working against development clusters, I often come across situations where a resource is misconfigured or completely missing. I've always found myself glad when it's just a misconfiguration because I can just fix the issue with the edit feature in k9s. However, when a resource is missing, it's much more disruptive. Switching to Breaking my flow to create a missing resource like this feels extra painful since the existing edit functionality almost does what would be needed (aside from the actual creation part). Note: I absolutely do not advocate cowboying like this on production clusters! I only work this way when working on development clusters… 😉😅
It is a bit rough starting from scratch, I agree. Duplicating sounds nice on paper, but the "gotchas" you describe would certainly make it treacherous to use…
Personally, I think this would make a ton of sense. Just having the basics like API group, version, kind, and namespace would go a long way to helping it feel better integrated. I could see an argument for a few other fields on a kind-by-kind basis, but I would be happy with just the basics 😄 |
|
I had a very quick look at dup and it seems that the plugin added in #2831 could provide the functionality for those who need it without adding it to the core of k9s. |
|
Nice addition, though canceling duplicate creation is missing. |
|
Closing as obsolete - there are at least two kubectl plugins that can be used for this by using them as k9s plugins, e.g. |
This PR is basically a rework of PR #954 which is coming originally from @NickyMateev - as there hasn't been any real activity by him on his original PR for the last 3 years and the functionality would be nice to have, I've rebased his work and streamlined it a bit.
Functionality in general is still the same, this PR introduces two new options:
kubectl createon the provided contents to create these resources.kubectl createon the contents once saved on closed.There is one key difference between the approach in #954 and this PR, in the original
kubectl applywas used while this PR switched tokubectl create. Why? Well, the answer is simple,createwill make sure that the user can't overwrite an existing resource simply by specifying the same name - for this the good old edit action has been around for a long time. This way, kubectl itself will act as a safeguard preventing users that simply forgot to change the name from messing up existing resources by either the create or duplicate actions.Also, this PR fixes the way the lookup of the preferred editor from the env vars
KUBE_EDITOR,K9S_EDITORand /EDITORworks. For most non-terminal editors, e.g. VSCode, one has to additionally pass an argument to prevent it from closing immediately, i.e.KUBE_EDITOR="code --wait". This broke the lookup, as k9s was trying to find a binary with the namecode --waiton the$PATH. This is now refactored to properly split any args from the binary to make sure we're really only looking for a binary name. Should hopefully also work on Windows, but I'm not able to test it myself.Readonly flag is supported, so both actions are not enabled if read-only flag is set.
On the original PR, @derailed noted that he doesn't like running imperative commands on a cluster to create resources without keeping the manifest on disk. For this PR, I simply decided to abuse the screen dumps dir and store a copy of the provided yaml file in there. The path is shown as part of the result of the create call.
Screenshots