Create and Duplicate Resource actions #2563
Conversation
nobbs
force-pushed
the
feat/resource-creation
branch
from
2e3a5d2
to
4da73bb
Compare
|
Fixed the failing test. I forgot to make use of the mock lookPath implementation for testing... 🙈 |
nobbs
force-pushed
the
feat/resource-creation
branch
2 times, most recently
from
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.
nobbs
force-pushed
the
feat/resource-creation
branch
from
0a72f43
to
0ba3bd2
Compare
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
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