derailed / k9s

🐶 Kubernetes CLI To Manage Your Clusters In Style!
https://k9scli.io
Apache License 2.0
27.56k stars 1.74k forks source link

Loading plugins from plugin directory not working as expected #2571

Closed norman-zon closed 3 months ago

norman-zon commented 9 months ago




Describe the bug As of this PR (included in >= v0.29) loading plugins from $XDG_DATA_DIRS/k9s/plugins/ instead of a $XDG_DATA_DIRS/k9s/plugins.yaml is supposed to work.

I can't get it to work though.

To Reproduce Steps to reproduce the behavior:

  1. create a directory $XDG_DATA_DIRS/k9s/plugins/
  2. add a file myplugins1.yaml
test1:
  shortCut: Shift-T
  scopes:
    - pods
  command: bash
  background: false
  args:
    - -c
    - "echo test1"

and myplugins2.yaml:

test2:
  shortCut: Shift-W
  scopes:
    - pods
  command: bash
  background: false
  args:
    - -c
    - "echo test2"
test3:
  shortCut: Shift-Q
  scopes:
    - pods
  command: bash
  background: false
  args:
    - -c
    - "echo test3"
  1. start k9s
  2. no plugins are available

Historical Documents

❯ k9s info
 ____  __.________
|    |/ _/   __   \______
|      < \____    /  ___/
|    |  \   /    /\___ \
|____|__ \ /____//____  >
        \/            \/

Version:           0.31.9
Config:            /Users/myuser/.config/k9s/config.yaml
Custom Views:      /Users/myuser/.config/k9s/views.yaml
Plugins:           /Users/myuser/.config/k9s/plugins.yaml
Hotkeys:           /Users/myuser/.config/k9s/hotkeys.yaml
Aliases:           /Users/myuser/.config/k9s/aliases.yaml
Skins:             /Users/myuser/.config/k9s/skins
Context Configs:   /Users/myuser/.local/share/k9s/clusters
Logs:              /Users/myuser/.local/state/k9s/k9s.log
Benchmarks:        /Users/myuser/.local/state/k9s/benchmarks
ScreenDumps:       /Users/myuser/.local/state/k9s/screen-dumps

Interesting that Plugins: /Users/myuser/.config/k9s/plugins.yaml is set

k9s -l debug gives no info on plugins failing to load

Expected behavior Plugins are loaded correctly

Versions:

wjiec commented 9 months ago

On macOS, XDG_DATA_DIRS by default points to /Library/Application Support instead of ~/Library/Application Support 🤔️. Also the plugin's config file should be in the following format:

# NO name
shortCut: Shift-T
scopes:
  - pods
command: bash
background: false
args:
  - -c
  - "echo test1"

I think this is a bit unintuitive, I think we should read the plugins from K9S_HOME, XDG_DATA_DIRS, XDG_DATA_HOME, and there should be support for plugins in the form of maps in each of these files (something like K8S_HOME/plugins.yaml). @derailed what do you think about this idea?

norman-zon commented 9 months ago

Thanks for the explanation @wjiec. I too think there should be support for maps in each file, to enable grouping. If I am forced to have a separate file per plugin it does not improve the handling and clearness in regard to having a single file. This would also help to bring the idea of integrating the community plugins tighter forward.

Regarding XDG_DATA_DIRS on MacOS, that was my oversight. But again, as you said reading all three directories seems to be the most intuitive solution.

derailed commented 9 months ago

@norman-zon @wjiec Thank you both for your feedback!

I am a bit torn on this deal ;( I think I understood Chris original intent on his PR ie managing plugins as separate files so they can be shared across an organization (via git or other) and may include either out-of-the-box or proprietary customizations. Hence keeping plugin files as single units make this task more manageable if plugin files as plugin snippets vs maps.

On the other hand, the typical flow (I think) is for a dev is to want to copy the plugins directly for this repo root and dump them into either a file or dirs.

It seems we are currently in a no man's land as this repo plugins dir exhibits plugin maps vs plugin snippets. My current incline would be to just convert to snippets and folks can cp/paste the entire file or content. The harder part here of course is when treated as units keeping the key mappings in good order makes it a bit rough.

I also think if we were to intro a k9s plugin-install add/delete sub command dealing with grouped plugins file will make this a tad trickier to manage vs dealing with single plugin units.

Does this make any sense?

Open for suggestions here if you have other inclines... Thank you!

/cc @cwrau

cwrau commented 9 months ago

On macOS, XDG_DATA_DIRS by default points to /Library/Application Support instead of ~/Library/Application Support 🤔️.

I'm confused, what's the problem? 🤔 It should use the value of the environment variable, if not you/we have to open an issue in github.com/adrg/xdg.

I think this is a bit unintuitive, I think we should read the plugins from K9S_HOME, XDG_DATA_DIRS, XDG_DATA_HOME, and there should be support for plugins in the form of maps in each of these files (something like K8S_HOME/plugins.yaml).

Huh, didn't expect this to be unintuitive, I would've assumed that support for multiple files meant a single plugin per file and the filename would be the plugin name, that's why I did it that way 😅

I am a bit torn on this deal ;( I think I understood Chris original intent on his PR ie managing plugins as separate files so they can be shared across an organization (via git or other) and may include either out-of-the-box or proprietary customizations.

Not just organizations or proprietary stuff, but every kubernetes tool, backup/database/gitops/..., can put stuff into these folders. It just spawned out of our company's need to share plugins.

My current incline would be to just convert to snippets and folks can cp/paste the entire file or content.

Yeah, that's what we'll be doing when we get to it, converting my plugins.yaml into multiple packages/providing the files upstream.

The harder part here of course is when treated as units keeping the key mappings in good order makes it a bit rough.

I don't remember, I think currently later defined plugins will completely overwrite the earlier definition, right? 🤔 Maybe we can do a merge of some kind, so you can overwrite specific fields, like the keybinds?

I also think if we were to intro a k9s plugin-install add/delete sub command dealing with grouped plugins file will make this a tad trickier to manage vs dealing with single plugin units.

What would such a command do? 🤔 Adding or deleting a plugin is as simple as creating/deleting a file.

derailed commented 9 months ago

@cwrau Thanks for weighting in! Makes sense. Plugins shortcut are indeed overwritten ie last load wins. As for introducing a sub command (linked in this issue) I do agree not much value there.

adamcharnock commented 9 months ago

This was a surprise to me too, I only figured it out after seeing this issue. I expected it to work in more of a conf.d kinda-way.

For anyone else, here is the ansible ended up using to install the cnpg plugin:

- name: Get the k9s cnpg plugin
  become: false
  run_once: true
  local_action:
    module: get_url
    url: "https://cloudnative-pg.io/documentation/{{cnpg_minor_version}}/samples/k9s/plugins.yml"
    dest: "/tmp/cnpg-plugins.yml"

- name: Install k9s cnpg plugin
  vars:
    plugins: "{{ (lookup('file', '/tmp/cnpg-plugins.yml')|from_yaml).plugins }}"
  with_items: "{{ plugins.keys() }}"
  copy:
    dest: "/usr/share/k9s/plugins/{{item}}.yml"
    content: "{{ plugins[item]|to_yaml }}"
cwrau commented 9 months ago

This was a surprise to me too, I only figured it out after seeing this issue. I expected it to work in more of a conf.d kinda-way.

Mh, I can understand that 🤔 I'm not totally opposed to changing this, but this would be a breaking change and therefore a major release, @derailed

For anyone else, here is the ansible ended up using to install the cnpg plugin:

- name: Get the k9s cnpg plugin
  become: false
  run_once: true
  local_action:
    module: get_url
    url: "https://cloudnative-pg.io/documentation/{{cnpg_minor_version}}/samples/k9s/plugins.yml"
    dest: "/tmp/cnpg-plugins.yml"

- name: Install k9s cnpg plugin
  vars:
    plugins: "{{ (lookup('file', '/tmp/cnpg-plugins.yml')|from_yaml).plugins }}"
  with_items: "{{ plugins.keys() }}"
  copy:
    dest: "/usr/share/k9s/plugins/{{item}}.yml"
    content: "{{ plugins[item]|to_yaml }}"

Tbh, they should include this file in their packages so the user can install them without hacks like this 😅

adamcharnock commented 9 months ago

I'm not totally opposed to changing this, but this would be a breaking change and therefore a major release

Yeah, I was thinking about that. Another option could be something like this:

  1. Parse the yaml
  2. If you see a plugins: key then parse it the new way
  3. Otherwise parse it the old way
  4. Put a deprecation warning on the old way