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

Add fish completion support #1516

Merged

Conversation

sayboras
Copy link
Contributor

@sayboras sayboras commented Apr 25, 2020

Description

Fixes #1303

Testing

asciicast

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 25, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @sayboras!

It looks like this is your first PR to kubernetes-sigs/kind 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kind has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 25, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @sayboras. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 25, 2020
@sayboras sayboras marked this pull request as draft April 25, 2020 23:19
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2020
@sayboras sayboras marked this pull request as ready for review April 26, 2020 01:29
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2020
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

Thanks for the PR

github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
github.com/NYTimes/gziphandler v0.0.0-20170623195520-56545f4a5d46/go.mod h1:3wb06e3pkSAbeQ52E9H9iFoQsEEwGN64994WTCIhntQ=
github.com/OneOfOne/xxhash v1.2.2/go.mod h1:HSdplMjZKSmBqAxg5vPj2TmRDmfkzw+cTzAElWljhcU=
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have so many new dependencies here?

Please run go mod tidy

Copy link
Contributor Author

@sayboras sayboras Apr 26, 2020

Choose a reason for hiding this comment

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

Yup, I ran go mod tidy only, seems like all these entries got added by go mod tidy.

$ go version 
go version go1.14.2 linux/amd64

$ go mod tidy

$ git status        
On branch feature/fish-completion
Your branch is up to date with 'origin/feature/fish-completion'.

nothing to commit, working tree clean

I noticed that go version in go.mod is with 1.13, however, make build is running with go 1.14.2.

Do you have any suggestion or idea ? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

1.13 is just a suggestion, that value isn't terribly meaningful in go.mod.
I'm seeing a lot of dependencies added here that I wouldn't expect, e.g google cloud and appengine 🤔

Copy link
Member

Choose a reason for hiding this comment

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

taking a look locally 😅

Copy link
Contributor Author

@sayboras sayboras Apr 26, 2020

Choose a reason for hiding this comment

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

oh i see, let me check out go mod graph and get back to you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh i got it cobra v1.0.0 requires github.com/spf13/viper v1.4.0 and github.com/spf13/[email protected] requires github.com/prometheus/[email protected]. This prometheus clieng golang dependency hell is kind of well known currently 😢

Copy link
Member

Choose a reason for hiding this comment

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

I think this is just go.sum not being very smart about what actual dependencies are 🙃

Sorry for the distraction.

I rermembered we can just make clean && make build and observe what packages are built in the verbose output:

internal/race
runtime/internal/sys
runtime/internal/atomic
internal/cpu
sync/atomic
runtime/internal/math
unicode
unicode/utf8
internal/bytealg
math/bits
internal/testlog
internal/nettrace
sigs.k8s.io/kind/pkg/log
math
runtime
encoding
unicode/utf16
internal/goversion
sigs.k8s.io/kind/pkg/apis/config/defaults
sigs.k8s.io/kind/pkg/cluster/constants
internal/reflectlite
sync
internal/singleflight
math/rand
sort
errors
io
internal/oserror
strconv
syscall
vendor/golang.org/x/net/dns/dnsmessage
bytes
reflect
bufio
strings
internal/syscall/execenv
time
internal/syscall/unix
path
regexp/syntax
context
internal/poll
golang.org/x/sys/unix
regexp
os
github.com/alessio/shellescape
encoding/binary
internal/fmtsort
k8s.io/apimachinery/pkg/util/sets
fmt
encoding/base64
vendor/golang.org/x/net/route
path/filepath
github.com/mattn/go-isatty
io/ioutil
sigs.k8s.io/kind/pkg/internal/env
os/exec
net
internal/lazyregexp
encoding/csv
encoding/hex
flag
encoding/json
net/url
text/template/parse
k8s.io/apimachinery/pkg/util/version
os/user
archive/tar
github.com/pkg/errors
text/template
k8s.io/apimachinery/pkg/util/errors
sigs.k8s.io/kind/pkg/errors
sigs.k8s.io/kind/pkg/exec
go/token
sigs.k8s.io/kind/pkg/build/nodeimage/internal/container/docker
go/scanner
internal/goroot
log
go/ast
sigs.k8s.io/kind/pkg/fs
sigs.k8s.io/kind/pkg/apis/config/v1alpha3
sigs.k8s.io/kind/pkg/apis/config/v1alpha4
github.com/spf13/pflag
sigs.k8s.io/kind/pkg/cluster/internal/loadbalancer
sigs.k8s.io/kind/pkg/cluster/nodes
sigs.k8s.io/kind/pkg/cluster/internal/logs
sigs.k8s.io/kind/pkg/internal/apis/config
sigs.k8s.io/kind/pkg/cluster/nodeutils
sigs.k8s.io/kind/pkg/cluster/internal/providers/provider/common
sigs.k8s.io/kind/pkg/cluster/internal/kubeadm
github.com/BurntSushi/toml
github.com/evanphx/json-patch/v5
go/doc
go/parser
math/big
gopkg.in/yaml.v3
go/build
sigs.k8s.io/kind/pkg/internal/cli
github.com/spf13/cobra
sigs.k8s.io/kind/pkg/cmd
sigs.k8s.io/kind/pkg/build/nodeimage/internal/kube
sigs.k8s.io/kind/pkg/build/nodeimage
sigs.k8s.io/kind/pkg/cluster/internal/providers/provider
github.com/pelletier/go-toml
sigs.k8s.io/kind/pkg/cluster/internal/providers/docker
sigs.k8s.io/kind/pkg/cmd/kind/build/nodeimage
sigs.k8s.io/kind/pkg/cmd/kind/completion/bash
sigs.k8s.io/kind/pkg/cmd/kind/completion/fish
sigs.k8s.io/kind/pkg/cmd/kind/build
sigs.k8s.io/kind/pkg/cmd/kind/completion/zsh
gopkg.in/yaml.v2
sigs.k8s.io/kind/pkg/cluster/internal/context
sigs.k8s.io/kind/pkg/cmd/kind/completion
sigs.k8s.io/kind/pkg/internal/apis/config/encoding
sigs.k8s.io/kind/pkg/cluster/internal/create/actions
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/installcni
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/installstorage
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/kubeadmjoin
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/kubeadminit
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/loadbalancer
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/waitforready
sigs.k8s.io/kind/pkg/cmd/kind/version
sigs.k8s.io/kind/pkg/cluster/internal/providers/podman
sigs.k8s.io/yaml
sigs.k8s.io/kind/pkg/cluster/internal/patch
sigs.k8s.io/kind/pkg/cluster/internal/kubeconfig/internal/kubeconfig
sigs.k8s.io/kind/pkg/cluster/internal/create/actions/config
sigs.k8s.io/kind/pkg/cluster/internal/kubeconfig
sigs.k8s.io/kind/pkg/cluster/internal/delete
sigs.k8s.io/kind/pkg/cluster/internal/create
sigs.k8s.io/kind/pkg/cluster
sigs.k8s.io/kind/pkg/internal/runtime
sigs.k8s.io/kind/pkg/cmd/kind/export/kubeconfig
sigs.k8s.io/kind/pkg/cmd/kind/create/cluster
sigs.k8s.io/kind/pkg/cmd/kind/delete/cluster
sigs.k8s.io/kind/pkg/cmd/kind/delete/clusters
sigs.k8s.io/kind/pkg/cmd/kind/export/logs
sigs.k8s.io/kind/pkg/cmd/kind/get/clusters
sigs.k8s.io/kind/pkg/cmd/kind/create
sigs.k8s.io/kind/pkg/cmd/kind/delete
sigs.k8s.io/kind/pkg/cmd/kind/get/kubeconfig
sigs.k8s.io/kind/pkg/cmd/kind/export
sigs.k8s.io/kind/pkg/cmd/kind/load/docker-image
sigs.k8s.io/kind/pkg/cmd/kind/get/nodes
sigs.k8s.io/kind/pkg/cmd/kind/load/image-archive
sigs.k8s.io/kind/pkg/cmd/kind/get
sigs.k8s.io/kind/pkg/cmd/kind/load
sigs.k8s.io/kind/pkg/cmd/kind
sigs.k8s.io/kind/cmd/kind/app
sigs.k8s.io/kind

filter those a bit:
$ cat packages.txt | grep -e '.' | grep -ve 'sigs.k8s.io/kind'
vendor/golang.org/x/net/dns/dnsmessage
golang.org/x/sys/unix
github.com/alessio/shellescape
vendor/golang.org/x/net/route
github.com/mattn/go-isatty
k8s.io/apimachinery/pkg/util/sets
k8s.io/apimachinery/pkg/util/version
github.com/pkg/errors
k8s.io/apimachinery/pkg/util/errors
github.com/spf13/pflag
github.com/BurntSushi/toml
github.com/evanphx/json-patch/v5
github.com/spf13/cobra
gopkg.in/yaml.v3
gopkg.in/yaml.v2
github.com/pelletier/go-toml
sigs.k8s.io/yaml

nothing new or surprising, and not most of these.

@@ -56,7 +56,8 @@ func NewCommand(logger log.Logger, streams cmd.IOStreams) *cobra.Command {
SilenceErrors: true,
Version: version.Version(),
}
cmd.SetOutput(streams.ErrOut)
cmd.SetOut(streams.Out)
cmd.SetErr(streams.ErrOut)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only breaking cobra change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, internally fish completion will run kind __complete <args> to capture the output used for completion. Hence, the current cmd.SetOutput(streams.ErrOut) will make stdout empty.

Copy link
Member

Choose a reason for hiding this comment

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

er sorry, to be clear: I meant with the large cobra version change specifically

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize we were so far behind (!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh it's not much changes from 0.0.5, only bug fixes and new feature for go custom completion. Actually, 0.0.5 is not so far befind, the maintainer of cobra feels it's good time to make it 1.0.0 version, the version before is 0.0.7.

FYI. The above SetOuput is still available, it's just deprecated. As I need to change output stream to std.Out for fish completion to work, I take this chance to use SetOut and SetErr methods as well.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

@BenTheElder
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 26, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2020
@BenTheElder
Copy link
Member

/lgtm
/approve
thank you! 😄

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, sayboras

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2020
@k8s-ci-robot k8s-ci-robot merged commit 0105e34 into kubernetes-sigs:master Apr 26, 2020
@BenTheElder BenTheElder added this to the v0.8.0 milestone Apr 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fish shell completion
3 participants