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 --verbose #243

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Fix --verbose #243

merged 2 commits into from
Dec 11, 2020

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Dec 11, 2020

We originally set the verbosity config after loading it from the --verbose flag. #225's changes to fix workspace issues grouped that config set with other config settings, before loading the --verbose flag value. As a result, deck always uses the default verbosity.

This change moves the --verbose flag load above those settings, restoring the ability to control verbosity.

Originally reported internally in FTI-2144. FWIW, no-color is still after everything in that block, but that doesn’t appear to matter/it’s still applied correctly in 1.2.3. Didn’t dig into how that works, but it doesn’t appear that needs a similar fix, so left it alone.

We originally set the verbosity config after loading it from the
--verbose flag. #225 grouped that config set with other config settings,
before loading the --verbose flag value. As a result, deck always used
the default verbosity.

This change moves the --verbose flag load above those settings,
restoring the ability to control verbosity.
@rainest rainest added the bug Something isn't working label Dec 11, 2020
@rainest rainest requested review from mflendrich and hbagdi December 11, 2020 00:37
@mflendrich
Copy link
Contributor

@rainest,

The issue with verbose was because #225 moved the assignment after the point where we use the value. This would've been prevented from happening if var verbose hadn't been declared earlier as a global variable.

verbose does not need to be a global variable, it makes sense to get rid of it.

Similarly, noColor is a global variable, and the flow of setting color.noColor is quite convoluted (value obtained in cobra.OnIntialize and then applied in PersistentPreRunE). I don't see a reason not to set color.noColor directly in the former place.

See 666ce73, which builds on top of this PR and streamlines handling of these parameters, that is:

  • removes an intermediate global var verbose (which was the cause of confusion leading to this bug),
  • removes an intermediate global var noColor and moves the color.NoColor assignment from PersistentPreRunE to the place where var noColor was previously initialized.

I recommend cherry-picking that commit into this PR if that looks good to you.

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2020

CLA assistant check
All committers have signed the CLA.

@mflendrich mflendrich merged commit 0886afd into main Dec 11, 2020
@mflendrich mflendrich deleted the fix/verbosity branch December 11, 2020 20:52
rainest pushed a commit that referenced this pull request Apr 21, 2021
* fix(cmd) restore verbosity

We originally set the verbosity config after loading it from the
--verbose flag. #225 grouped that config set with other config settings,
before loading the --verbose flag value. As a result, deck always used
the default verbosity.

This change moves the --verbose flag load above those settings,
restoring the ability to control verbosity.

* refactor(flags): simplify verbose and noColor

Co-authored-by: Michał Flendrich <michal@flendrich.pro>
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
* fix(cmd) restore verbosity

We originally set the verbosity config after loading it from the
--verbose flag. #225 grouped that config set with other config settings,
before loading the --verbose flag value. As a result, deck always used
the default verbosity.

This change moves the --verbose flag load above those settings,
restoring the ability to control verbosity.

* refactor(flags): simplify verbose and noColor

Co-authored-by: Michał Flendrich <michal@flendrich.pro>
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.

3 participants