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

hotfix for ignored --env flag issue (#269) #727

Merged
merged 4 commits into from Jun 30, 2022
Merged

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Jun 4, 2022

Not fully sure if the issue happen again or if the previous patch does not fill all the cases. Anyway, the same issue happens again:

$ soda reset -e test
pop v6.0.2

[POP] 2022/06/04 20:40:03 info - drop app_development (...)
Error: couldn't drop database app_development: error dropping...

Issue Description

Even though the --env (or short -e) flag is given, the command just follows the environment variable.

Root Cause

It seems like the issue was caused by confusing naming and the behavior of PersistantPreRun field of cobra.Command and PersistentFlags() function. I added a detailed comment on the code so easy to check later.

(One unrelated commit is also included :-)

Related: #282
Fixes #269

@sio4 sio4 added the bug Something isn't working label Jun 4, 2022
@sio4 sio4 requested review from a team June 4, 2022 14:45
@sio4 sio4 self-assigned this Jun 4, 2022
@sio4 sio4 enabled auto-merge (rebase) June 4, 2022 14:46
soda/cmd/root_integration_test.go Outdated Show resolved Hide resolved
soda/cmd/root_integration_test.go Show resolved Hide resolved
soda/cmd/root_integration_test.go Show resolved Hide resolved
@sio4
Copy link
Member Author

sio4 commented Jun 28, 2022

For the modified test, the original test was passed successfully before even though there was an issue of ignoring flag provided environment since the flag parsing mechanism is somewhat tricky.

  • soda --env production --> env: production, test passed since the flag was considered
  • soda command --env production --> env: test (from envrionment), flag was ignored
  • soda --env production command --> env: test (from environment), flag was ignored

For the first case, the effective command was the root command since there is no sub-command argument, and the flag was interpreted successfully because it is the effective command's flag. However, for the other cases, the flag is still the root's flag but the effective command is the sub-command, and they are not checked. (This is the reason I added "help" to the test case)

This issue might be loosely related to the issue gobuffalo/cli#167. The interpretation order for flags (both global and per sub-command) and sub-commands is not consistent and it made unexpectable behavior.

I think we need to reconsider this issue with wider use cases once we fixed the plugin architecture. (I heard that @paganotoni is working on plugin architecture for the buffalo CLI)

@sio4 sio4 requested a review from stanislas-m June 28, 2022 15:24
@sio4
Copy link
Member Author

sio4 commented Jun 28, 2022

Hi @stanislas-m! Thanks for the comment.

I added the explanation of the changed test more clearly (I think :-) and added commits to fix

  • the typo on the test name, and more test condition
  • fix for the Github action (recent windows runner has a memory allocation issue when race detector is on)

Please take a look at the commits and the last comment I added.

Thanks!

@sio4 sio4 merged commit d15b0f9 into development Jun 30, 2022
@sio4 sio4 deleted the hotfix-env-flag-issue branch June 30, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants