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

Adding skim support #312

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Adding skim support #312

wants to merge 3 commits into from

Conversation

frzam
Copy link

@frzam frzam commented Aug 8, 2021

See the Feature request here

Added skim support in kubens and kubectx go version.

Added environment variable PICKER which will contain value sk or fzf, if PICKER is not set or anything else is set other than sk and fzf, then it will by default pick fzf.

Created a func InteractiveSearch in interactive.go which will be called for fuzzy search and renamed fzf.go to fuzzy.go since it is now more generic.

PICKER env variable can have values (sk or fzf), if variable is not set,
then fzf is used as picker.
Like KUBECTX_IGNORE_FZF we have created one more KUBECTX_IGNORE_SK which
will be used to ignore sk.
Renamed fzf.go to fuzzy.go since it is now more generic, refactored code
to launch fuzzy search from func InteractiveSearch in
interactive.go since it used by both kubens and kubectx
@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@frzam
Copy link
Author

frzam commented Aug 8, 2021

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 8, 2021
@ahmetb
Copy link
Owner

ahmetb commented Aug 30, 2021

Hey, sorry I've been slow reviewing things!
For compatibility purposes, do you mind if we check fzf first, then sk? I am suspecting if people have another binary somehow called sk in their PATH, their workflow might be broken. (And this can work because I assume sk users don't have fzf installed?)

@@ -29,4 +29,12 @@ const (

// EnvDebug describes the internal environment variable for more verbose logging.
EnvDebug = `DEBUG`

// EnvPicker describes the environment variable for fuzzy support, It can value
// fzf or sk. If this is not set then fzf is taken as default picker.
Copy link
Owner

Choose a reason for hiding this comment

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

If this is not set then fzf is taken as default picker.

My expectation of this feature was that people can set this to anything and we can feature-proof the tool. e.g. imagine a tool named abc is developed in the future, I should be able to say PICKER=abc --opt1 -opt2.

I think this is how more common env vars like EDITOR or PAGER in posix systems work. The caller has no information about the program it's calling other than it knows how to pass it the information (either via xargs or via stdin).

Copy link
Owner

Choose a reason for hiding this comment

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

Also this way, we can retain fzf logic and have an override via PICKER. The source code does not need to know about sk at all.

@sambonbonne
Copy link

sambonbonne commented Apr 11, 2022

For those who are waiting for this PR to be merged, you can create an alias with kubectx $(kubectx | sk), it's not a pefect solution but it works.

EDIT: typo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants