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: pre-select stuff with fzf #324

Closed
wants to merge 2 commits into from
Closed

feat: pre-select stuff with fzf #324

wants to merge 2 commits into from

Conversation

caarlos0
Copy link
Contributor

as discussed on twitter (https://twitter.com/caarlos0/status/1443410098471194625), PR adding this feature.

  • if input is -, switch right away
  • if input is interactive, go through fzf with input as query
  • if not interactive, switch as before

implemented on both kubectx and kubens.

PS: would you also be interested in having a homebrew tap that uses the go versions? Right now I'm maintaining my own, but I can port the config here if you create a homebrew-tap repository :)

Cheers!

Signed-off-by: Carlos Alexandro Becker <[email protected]>
@ahmetb
Copy link
Owner

ahmetb commented Oct 1, 2021

Some questions

  1. How do we define this in the CLI --help so that people can tell this is the behavior without having to read the readme? (right now you don't need to read the readme to learn how kubectx works)

  2. Should/does this only work if stdout is a TTY (i.e. an interactive shell prompt)? I assume this is not a desirable behavior for scripts etc (though people shouldnt use kubectx for scripts, some unfortunately do)?

  3. How/whether can we retain the same behavior if we allow customization to let users use other fuzzy-finder tools through a variable like $PICKER (e.g. Adding skim support #312)?

@caarlos0
Copy link
Contributor Author

caarlos0 commented Oct 2, 2021

  • How do we define this in the CLI --help so that people can tell this is the behavior without having to read the readme? (right now you don't need to read the readme to learn how kubectx works)

hmm, good question... would have to make it a bit more convoluted to explain it properly I believe...

  • Should/does this only work if stdout is a TTY (i.e. an interactive shell prompt)? I assume this is not a desirable behavior for scripts etc (though people shouldnt use kubectx for scripts, some unfortunately do)?

Currently its using the same check it does for other fzf commands... so, yea, only on interactive prompts.

  • How/whether can we retain the same behavior if we allow customization to let users use other fuzzy-finder tools through a variable like $PICKER (e.g. Adding skim support #312)?

I think that highly depends on the fuzzy-finder... but specific to skim, judging by their docs, it seems it would work...

cmd/kubens/flags.go Outdated Show resolved Hide resolved
cmd/kubens/flags.go Outdated Show resolved Hide resolved
@@ -50,7 +51,7 @@ func (op InteractiveSwitchOp) Run(_, stderr io.Writer) error {
}
kc.Close()

cmd := exec.Command("fzf", "--ansi", "--no-preview")
cmd := exec.Command("fzf", "--ansi", "--no-preview", "-q", op.Target, "-1")
Copy link
Owner

@ahmetb ahmetb Oct 15, 2021

Choose a reason for hiding this comment

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

lets type out --query and --select-1 explicitly, it's better for readability.
(ditto for kubens)

Copy link
Owner

Choose a reason for hiding this comment

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

also to be clear, -q "" works as we expect, right?

we could always do:

if query != "" {
  args=append(args, "-q",query)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also to be clear, -q "" works as we expect, right?

yes it does, it passes an empty string, so it'll show the fzf screen as before...

Copy link
Owner

Choose a reason for hiding this comment

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

ok just wanted to make sure we're not relying on undocumented behavior (empty string vs unset)

cmd/kubens/fzf.go Outdated Show resolved Hide resolved
@ahmetb
Copy link
Owner

ahmetb commented Oct 15, 2021

I think that highly depends on the fuzzy-finder... but specific to skim, judging by their docs, it seems it would work...

yeah I think we can live without other fuzzy-finder supporting this feature (and can WARN the user if we don't know how to pass this arg down in case $FINDER is set)

Signed-off-by: Carlos A Becker <[email protected]>
@ahmetb
Copy link
Owner

ahmetb commented Oct 15, 2021

something to note is that when/if we introduce a custom $PICKER support, we now have to find a way that users can indicate to pass this $QUERY option in that value. e.g.:

export PICKER="my_picker --arg1 --arg2 _"

and \b_\b would get replaced with the query. Up until now, we didn't have this problem but it's good that we're coming up with this requirement sooner than later down the road.

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.

3 participants