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 flag completion #1438

Merged
merged 2 commits into from Jul 2, 2021
Merged

Fix flag completion #1438

merged 2 commits into from Jul 2, 2021

Conversation

@Luap99
Copy link
Contributor

@Luap99 Luap99 commented 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 #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

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

@jpmcb @marckhouzam PTAL

Every program who calls cmd.RegisterFlagCompletionFunc() before joining the cmd to the root command will no longer have functional flag completion. I think it will break many applications since it is common to add flags to the cmd before adding it the root/parent cmd.
It would be great if we can get this fixed soon.

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

OK this patch doesn't work either. Storing them in the correct cmd doesn't work for persistent flags.

I also do not understand what #1423 is supposed to fix. The map is just stored in the root cmd now, writing concurrently will still fail.

@Luap99 Luap99 force-pushed the fix-1437 branch 2 times, most recently from 41a0a4e to 16378fa Jul 2, 2021
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

OK, this should work now and truly fix the concurrency problem.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Thanks for the quick reaction @Luap99 !

This look good with a couple of nits.

However, I haven't tested if it really fixes the concern of istioctl mentioned in #1423 (comment), but I expect it would.

completions.go Outdated Show resolved Hide resolved
completions_test.go Outdated Show resolved Hide resolved
completions_test.go Outdated Show resolved Hide resolved
@marckhouzam
Copy link
Contributor

@marckhouzam marckhouzam commented Jul 2, 2021

@silenceshell are you able to test if this fix addresses the concern you were aiming to fix for istioctl in #1423 (comment)?

@silenceshell
Copy link
Contributor

@silenceshell silenceshell commented Jul 2, 2021

@silenceshell are you able to test if this fix addresses the concern you were aiming to fix for istioctl in #1423 (comment)?

This PR could fix the istioctl test cases, RWMutex is safe enough.

@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

@marckhouzam Updated.

Luap99 added 2 commits 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>
Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@silenceshell
Copy link
Contributor

@silenceshell silenceshell commented Jul 2, 2021

OK this patch doesn't work either. Storing them in the correct cmd doesn't work for persistent flags.

I also do not understand what #1423 is supposed to fix. The map is just stored in the root cmd now, writing concurrently will still fail.

Sorry for the incomplete consideration.
Istio has some tests that call rootCmd reg func concurrently, #1423 wants to store the map to rootCmd to avoid concurrent map writes errors. This could fix that problem but breaks the current implement. sorry for this again, and thanks for the correction.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

This looks good.
I was able to reproduce the regression with Helm, and can confirm this PR fixes things.
I had not seen the regression because most of helm flags that use completion are on the root command.

Thanks @Luap99

@jpmcb what do you think for a 1.2.1 release?

jpmcb
jpmcb approved these changes Jul 2, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

Merging to get a quick fix out for this regression. Thanks all for the fast find and quick testing efforts on this!

@jpmcb jpmcb merged commit de187e8 into spf13:master Jul 2, 2021
7 checks passed
@Luap99 Luap99 deleted the fix-1437 branch Jul 2, 2021
@Luap99
Copy link
Contributor Author

@Luap99 Luap99 commented Jul 2, 2021

@jpmcb Thanks for the quick review and the new release tag.

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 issues

Successfully merging this pull request may close these issues.

4 participants