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

feat: allow for multiple plugin files in $XDG_DATA_DIRS/k9s/plugins #2029

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

cwrau
Copy link
Contributor

@cwrau cwrau commented Mar 23, 2023

Closes #2028

@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from dfbac9e to 74e1255 Compare March 23, 2023 16:01
@cwrau
Copy link
Contributor Author

cwrau commented Mar 23, 2023

Tests will follow 👍️

internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/plugin.go Outdated Show resolved Hide resolved
@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 74e1255 to 7f4d0af Compare March 24, 2023 16:21
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwrau First off thank you so much Chris for this pr!
I really like the idea and especially the concept here in terms of sharing configurations across for all of us to use. This could extend to other artifacts beside plugins as well...
That said, I am not keen on the mechanics of the system wide location as this feels a bit as a departure from the current spec.
K9s uses xdg[https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html] to track its artifacts and I think it would be beneficial to continue on this directive. For instance we could leverage XDG_DATA_DIRS to load extra configs if set. This would afford from allowing to extends k9s with plugins, themes, etc... that could be shared org wide.
One other thing this makes me think about, is instead of canning the plugins in an install tar ball, one could create a git repo to share these artifacts with co-workers/community folks as either public/private repos. This also provides a better upgrade path from single files to dirs.
Would this make better sense?

@cwrau
Copy link
Contributor Author

cwrau commented Apr 13, 2023

@cwrau First off thank you so much Chris for this pr! I really like the idea and especially the concept here in terms of sharing configurations across for all of us to use. This could extend to other artifacts beside plugins as well... That said, I am not keen on the mechanics of the system wide location as this feels a bit as a departure from the current spec. K9s uses xdg[https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html] to track its artifacts and I think it would be beneficial to continue on this directive. For instance we could leverage XDG_DATA_DIRS to load extra configs if set. This would afford from allowing to extends k9s with plugins, themes, etc... that could be shared org wide.

Sure, I can incorporate the XDG_DATA_DIRS, but then I'd remove the default /usr/lib/k9s/plugins, as the default for XDG_DATA_DIRS handles /usr/share/k9s/plugins, so by default the user doesn't have to do anything.

I choose /usr/lib/k9s/plugins because the author of the AUR package of helm-diff decided against /usr/share and used /usr/lib/helm/plugins.
His argument is, that these files aren't architecture-independent, though I don't know why that matters 😅

Maybe @ml- can shed some light on his reasoning?

But in any way, our stuff isn't compiled, therefore /usr/share is just fine 👍️

One other thing this makes me think about, is instead of canning the plugins in an install tar ball, one could create a git repo to share these artifacts with co-workers/community folks as either public/private repos. This also provides a better upgrade path from single files to dirs. Would this make better sense?

What do you mean by canning the plugins in an install tar ball? 🤔
If you refer to potential AUR packages, they are all just git repositories which can be used manually as well and has the upside of being managed by the system package manager.

@ml-
Copy link

ml- commented Apr 14, 2023

Back in 2020 when I created AUR packages for helm plugins, there was no existing package to use as reference and system wide installation of a plugin via package manager was not a use-case.

So I consulted the ArchWiki on what the (more) correct place is for the files.

Table over Arch_package_guidelines#Directories had the following hint:

/usr/lib/pkg Modules, plugins, etc.

file-hierarchy (7) and Filesystem Hierarchy Standard have further information on what files are expected in /usr/share and /usr/lib/....
I haven't touched this topic for a longer time and would definitly reevaluate the decision I made back then.

XDG_DATA_DIRS (/usr/share) looks like a good choice here.

@cwrau
Copy link
Contributor Author

cwrau commented Apr 14, 2023

Back in 2020 when I created AUR packages for helm plugins, there was no existing package to use as reference and system wide installation of a plugin via package manager was not a use-case.

So I consulted the ArchWiki on what the (more) correct place is for the files.

Table over Arch_package_guidelines#Directories had the following hint:

/usr/lib/pkg Modules, plugins, etc.
file-hierarchy (7) and Filesystem Hierarchy Standard have further information on what files are expected in /usr/share and /usr/lib/.... I haven't touched this topic for a longer time and would definitly reevaluate the decision I made back then.

XDG_DATA_DIRS (/usr/share) looks like a good choice here.

Thanks for your input!

I agree, XDG_DATA_DIRS seems like an easy general solution 👍️

I'm gonna implement that 👌

@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 7f4d0af to 26bf45b Compare April 14, 2023 08:15
@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 26bf45b to 9e02816 Compare April 21, 2023 11:33
@cwrau
Copy link
Contributor Author

cwrau commented Apr 21, 2023

So, this should be about ready 👍️

@cwrau cwrau requested a review from derailed May 4, 2023 10:57
@cwrau
Copy link
Contributor Author

cwrau commented May 5, 2023

@derailed, is there something I can do to get this rolling? 😁

@derailed
Copy link
Owner

derailed commented May 6, 2023

@cwrau Did you see my review comments?

@cwrau
Copy link
Contributor Author

cwrau commented May 6, 2023

@cwrau Did you see my review comments?

Mh, not really 🤔

I've been trying to look at them but I can't find a way to show them.

When I click on "view review", it just shows your comment.

I thought that your requested changes were already superseded 😅

@derailed
Copy link
Owner

derailed commented May 7, 2023

@cwrau Tx Chris! Sorry I am missing what you mean ;(
Are you not seeing my review comments or you have fixes for them that I am somehow not seeing?
I've looked but I don't see any updates/resolved from my PR review. What did I miss?

@cwrau
Copy link
Contributor Author

cwrau commented May 8, 2023

@cwrau Tx Chris! Sorry I am missing what you mean ;( Are you not seeing my review comments or you have fixes for them that I am somehow not seeing? I've looked but I don't see any updates/resolved from my PR review. What did I miss?

I must be missing something, I can't for the life of me figure out how to view your requested changes 😅

See the attached video, I must be missing something 🥺

google_screen_recording_2023-05-08T08-50_20.529Z.webm

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwrau Doh! it would have helped if I had pressed enter here. My bad ;( with feelings...

internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/plugin.go Outdated Show resolved Hide resolved
internal/config/plugin.go Outdated Show resolved Hide resolved
@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 9e02816 to ff00994 Compare May 9, 2023 09:30
@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from ff00994 to 435cbd0 Compare May 17, 2023 07:43
@cwrau
Copy link
Contributor Author

cwrau commented May 27, 2023

@derailed I've resolved all your comments, is there something else to do? 😁

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwrau Tx for the updates! Just a minor pick..

internal/config/plugin.go Outdated Show resolved Hide resolved
@cwrau cwrau changed the title feat: allow for multiple plugin files in /usr/lib/k9s/plugins feat: allow for multiple plugin files in $XDG_DATA_DIRS/k9s/plugins May 30, 2023
@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 435cbd0 to 30b2057 Compare May 30, 2023 08:23
@cwrau
Copy link
Contributor Author

cwrau commented Jun 12, 2023

👋😉

@cwrau cwrau force-pushed the feat/allow-multiple-plugin-files branch from 30b2057 to 6aafb81 Compare June 12, 2023 13:56
Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwrau Thanks for the corrections Chris!

@cwrau
Copy link
Contributor Author

cwrau commented Jul 25, 2023

Heyho, is there a timeline for when we can merge this? 😁

I'm eagerly awaiting this feature!

@cwrau
Copy link
Contributor Author

cwrau commented Aug 24, 2023

PingPong, just wanted to ask if there is something I can do to get this merged?

Copy link
Owner

@derailed derailed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwrau Thanks for these updates Chris!

@derailed derailed added the enhancement New feature or request label Nov 12, 2023
@derailed derailed merged commit 1bfd824 into derailed:master Nov 12, 2023
rm-hull added a commit to rm-hull/k9s that referenced this pull request Nov 12, 2023
* 'master' of github.com:derailed/k9s: (130 commits)
  added flux suspended resources retrieval plugin (derailed#1584)
  Provide white blur so images work in dark modes (derailed#1597)
  Add context to get-all (derailed#1701)
  fix brew command in the readme (derailed#2012)
  Add support for using custom kubeconfig with log_full plugin (derailed#2014)
  feat: allow for multiple plugin files in $XDG_DATA_DIRS/k9s/plugins (derailed#2029)
  Clean up issues introduced by derailed#2125 (derailed#2289)
  Pod view resembles more the output of kubectl get pods -o wide (derailed#2125)
  Update README.md with snap install (derailed#2262)
  Add snapcraft config (derailed#2123)
  storageclasses view keeps the same output as kubectl get sc (derailed#2132)
  Fix merge issues with PR derailed#2168 (derailed#2288)
  Add colour config for container picker (derailed#2140)
  Add env var to disable node pod counts (derailed#2168)
  Use current k9s NS if new context has no default NS (derailed#2197)
  Bump actions/setup-go from 4.0.1 to 4.1.0 (derailed#2200)
  fix: trigger a single log refresh after changing 'since' (derailed#2202)
  Add crossplane plugin (derailed#2204)
  fix(derailed#1359): add option to keep missing clusters in config (derailed#2213)
  K9s release v0.28.2
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin directory instead of a single file
4 participants