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

Allow --verbose, --debug and --allow-self-signed everywhere #411

Merged
merged 3 commits into from Nov 18, 2022

Conversation

agateau-gg
Copy link
Collaborator

Description

Click does not allow defining options across groups, this means ggshield -v secret scan path foo.py works but not ggshield secret scan path foo.py -v.

What has been done

This PR introduces a new module in cmd: common_options. This module provides an add_common_options() decorator to define Click options we want to make available anywhere on the command-line. Supported options are:

  • -v, --verbose
  • --debug
  • --allow-self-signed

With these changes, all these commands now work and do the same thing:

ggshield -v secret scan path foo.py
ggshield secret -v scan path foo.py
ggshield secret scan -v path foo.py
ggshield secret scan path -v foo.py
ggshield secret scan path foo.py -v

In the next episode PR

I have another branch stacked on top of this one to do something similar for secret scan options like --json, --exit-zero, --show-secrets...

Issue

Part of #197

Copy link
Collaborator

@pierrelalanne pierrelalanne left a comment

Choose a reason for hiding this comment

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

Looks great. I have two general remarks :

  • We should be careful to add these common_options to future commands when we add them : should we document this ?
  • Do you think we should add some kind of functional testing on these options' positions ? I think we should at least write the exhaustive list of commands to QA (which may be super long...)
    WDYT ?

ggshield/cmd/main.py Outdated Show resolved Hide resolved
ggshield/cmd/common_options.py Show resolved Hide resolved
@agateau-gg
Copy link
Collaborator Author

  • We should be careful to add these common_options to future commands when we add them : should we document this ?

I agree with the idea, but I don't know a reliable place to document it, any suggestions? On the other hand cargo-culting is likely to prevent issues :)

  • Do you think we should add some kind of functional testing on these options' positions ? I think we should at least write the exhaustive list of commands to QA (which may be super long...)

Testing all possible combinations sounds overkill. Maybe modifying our tests to use different positions in different tests would be good enough?

@agateau-gg agateau-gg force-pushed the agateau/verbose-everywhere branch 2 times, most recently from bf5610b to cf0b12f Compare November 9, 2022 14:28
@pierrelalanne
Copy link
Collaborator

I agree with your second point :

Testing all possible combinations sounds overkill. Maybe modifying our tests to use different positions in different tests would be good enough?

Adding some variations in the functional tests looks wise.

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022

Codecov Report

Merging #411 (a9e22b0) into main (f4cbc23) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #411      +/-   ##
==========================================
+ Coverage   94.27%   94.49%   +0.21%     
==========================================
  Files          80       82       +2     
  Lines        3425     3521      +96     
==========================================
+ Hits         3229     3327      +98     
+ Misses        196      194       -2     
Flag Coverage Δ
unittests 94.49% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
ggshield/cmd/auth/__init__.py 100.00% <100.00%> (ø)
ggshield/cmd/auth/login.py 97.05% <100.00%> (+0.08%) ⬆️
ggshield/cmd/auth/logout.py 100.00% <100.00%> (ø)
ggshield/cmd/common_options.py 100.00% <100.00%> (ø)
ggshield/cmd/config/__init__.py 100.00% <100.00%> (ø)
ggshield/cmd/config/config_get.py 92.59% <100.00%> (+0.59%) ⬆️
ggshield/cmd/config/config_list.py 100.00% <100.00%> (ø)
ggshield/cmd/config/config_migrate.py 100.00% <100.00%> (ø)
ggshield/cmd/config/config_set.py 100.00% <100.00%> (ø)
ggshield/cmd/config/config_unset.py 100.00% <100.00%> (ø)
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@agateau-gg
Copy link
Collaborator Author

I agree with your second point :

Testing all possible combinations sounds overkill. Maybe modifying our tests to use different positions in different tests would be good enough?

Adding some variations in the functional tests looks wise.

Just did this. Going to merge when CI is happy.

@agateau-gg agateau-gg merged commit 71c3e74 into main Nov 18, 2022
@agateau-gg agateau-gg deleted the agateau/verbose-everywhere branch November 18, 2022 16:36
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