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

Fix bash completion bugs #280

Merged
merged 7 commits into from Jul 13, 2023
Merged

Fix bash completion bugs #280

merged 7 commits into from Jul 13, 2023

Conversation

dnr
Copy link
Member

@dnr dnr commented Jul 6, 2023

What was changed

Why?

Makes completing deep subcommands work correctly and easily

Checklist

  1. Closes

  2. How was this tested:
    manually

  3. Any docs updates needed?

@dnr
Copy link
Member Author

dnr commented Jul 7, 2023

Actually I realized we're using v1 of the bash completion script: https://github.com/urfave/cli/blob/master/autocomplete/bash_autocomplete
v2/v3 looks slightly different and does depend on the bash-completion library: https://github.com/urfave/cli/blob/v2-maint/autocomplete/bash_autocomplete

Apparently this fixes bugs with args with colons and quotes: urfave/cli#1674

Is there an intention to move to v2/v3 of the script? Fixing bugs is nice but it works on commands, which is the major win, and the dependency is annoying, since I want to enable this in the admin-tools image (there is an alpine package though so it could be done).

@@ -15,7 +15,7 @@ require (
github.com/stretchr/testify v1.8.4
github.com/temporalio/tctl-kit v0.0.0-20230328153839-577f95d16fa0
github.com/temporalio/ui-server/v2 v2.16.2
github.com/urfave/cli/v2 v2.23.6
github.com/urfave/cli/v2 v2.25.7
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if this is no longer the case for urfave/cli > v2.23.6 https://github.com/temporalio/cli/blob/d37f65cec42f708370ec06d3cf92088ce71eb827/Makefile#L20C147-L20C147

Since we now have e2e setup i'll likely rewrite this test with an actual workflow input

func (s *cliAppSuite) TestAcceptStringSliceArgsWithCommas() {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.. I just tested it and it seems to work. Should I delete the pinned dependency?

Copy link
Member Author

@dnr dnr left a comment

Choose a reason for hiding this comment

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

Any thoughts on the dependency and script version? I'm tempted to leave it on the old script and live with the bugs in exchange for no dependencies

@@ -15,7 +15,7 @@ require (
github.com/stretchr/testify v1.8.4
github.com/temporalio/tctl-kit v0.0.0-20230328153839-577f95d16fa0
github.com/temporalio/ui-server/v2 v2.16.2
github.com/urfave/cli/v2 v2.23.6
github.com/urfave/cli/v2 v2.25.7
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm.. I just tested it and it seems to work. Should I delete the pinned dependency?

@feedmeapples
Copy link
Contributor

Any thoughts on the dependency and script version? I'm tempted to leave it on the old script and live with the bugs in exchange for no dependencies

We may move to cobra and its auto-completion does depend on bash-completion. git relies on this package too. I vote it's fine to add dependency for improved UX in bash

@dnr
Copy link
Member Author

dnr commented Jul 7, 2023

We may move to cobra and its auto-completion does depend on bash-completion. git relies on this package too. I vote it's fine to add dependency for improved UX in bash

Ok, I'll update the script and revert the README changes

@dnr dnr merged commit 30a73b2 into main Jul 13, 2023
15 checks passed
@dnr dnr deleted the david/completion1 branch July 13, 2023 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants