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

1.2.0 regression: flag completion issues #1437

Closed
Luap99 opened this issue Jul 2, 2021 · 2 comments
Closed

1.2.0 regression: flag completion issues #1437

Luap99 opened this issue Jul 2, 2021 · 2 comments

Comments

@Luap99
Copy link
Contributor

@Luap99 Luap99 commented Jul 2, 2021

PR #1423 changed the location where flag completion functions are stored. It always stores them under the root cmd. This breaks Podman because we setup each cobra cmd struct individually with their flags before we join them to the root command.
So when we call cmd.RegisterFlagCompletionFunc() the root cmd will not be the actual root cmd. When the __complete command tries to look up flag completions it will not find anything in the real root cmd and thus no completion is provided.

ref containers/podman#10841

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

Every program who calls cmd.RegisterFlagCompletionFunc() before joining the cmd to the root command will no longer have functional flag completion.

Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should be stored on their cmd struct and
not the root cmd. Using the root cmd does not work when you create the
cmd and add flags before you join this cmd struct to your parent command.

Fixes spf13#1437

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should be stored on their cmd struct and
not the root cmd. Using the root cmd does not work when you create the
cmd and add flags before you join this cmd struct to your parent command.

Fixes spf13#1437

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Jul 2, 2021

Shoot, I didn't think about such a scenario, although it is pretty common. Sorry about that.

Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Luap99 added a commit to Luap99/cobra that referenced this issue Jul 2, 2021
The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also spf13#1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes spf13#1437
Fixes spf13#1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@jpmcb jpmcb closed this in #1438 Jul 2, 2021
jpmcb pushed a commit that referenced this issue Jul 2, 2021
* Fix flag completion

The flag completion functions should not be stored in the root cmd.
There is no requirement that the root cmd should be the same when
`RegisterFlagCompletionFunc` was called. Storing the flags there does
not work when you add the the flags to your cmd struct before you add the
cmd to the parent/root cmd. The flags can no longer be found in the rigth
place when the completion command is called and thus the flag completion
does not work.

Also #1423 claims that this would be thread safe but we still have a map
which will fail when accessed concurrently. To truly fix this issue use a
RWMutex.

Fixes #1437
Fixes #1320

Signed-off-by: Paul Holzinger <pholzing@redhat.com>

* Fix trailing whitespaces in fish comp scripts

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
gcf-merge-on-green bot pushed a commit to googleapis/gapic-showcase that referenced this issue Jul 2, 2021
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github.com/spf13/cobra](https://togithub.com/spf13/cobra) | require | patch | `v1.2.0` -> `v1.2.1` |

---

### Release Notes

<details>
<summary>spf13/cobra</summary>

### [`v1.2.1`](https://togithub.com/spf13/cobra/releases/v1.2.1)

[Compare Source](https://togithub.com/spf13/cobra/compare/v1.2.0...v1.2.1)

##### Bug fixes

-   Quickfix for spf13/cobra#1437 after v1.2.0 where parallel use of the `cmd.RegisterFlagCompletionFunc()` (and subsequent map) now works correctly and flag completions now work again

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/gapic-showcase).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants