-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Create and Duplicate Resource actions #2563
base: master
Are you sure you want to change the base?
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
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 create
on the provided contents to create these resources.kubectl create
on the contents once saved on closed.There is one key difference between the approach in #954 and this PR, in the original
kubectl apply
was used while this PR switched tokubectl create
. Why? Well, the answer is simple,create
will 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_EDITOR
and /EDITOR
works. 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 --wait
on 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