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

Option flags #19

Merged
merged 10 commits into from Jan 11, 2019
Merged

Option flags #19

merged 10 commits into from Jan 11, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 10, 2019

Cleanup, adds test

  1. Renamed is_rate_limited to sleep_if_needed to better clarify
    behavior
  2. Extract rate limiting error to a module constant
  3. Add test suite and required dependencies
    Run 'nosetests' from your virtual environment to see it in action
  4. Wrapped long lines

Ronald Dahlgren and others added 9 commits January 9, 2019 18:59
Resolves #6

This commit adds flags for each of the specific scans. If the `--no-foo`
option is provided, then the `foo_scan` function is not executed. The
collection of scans and flags are kept in an array of tuples for easy
maintenance.
Adds flags to selectively disable scans
Add flags to enable/disable scans
I renamed some of the variables so that they are easier (for me anyway) to understand.

Also removed the `if (anyTrue and anyFalse)` as that condition will actually never be hit as specifying --no-flag and --flag <--- the latter flag will take precedence.

It's easier to just remove the check completely but if you guys think it's worth doing a check for contradictory flags then we can do that too but it could get complicated.

Otherwise the default behaviour will mean that the 'latest' flag gets set and I don't mind that.

Thoughts @milangfx @influenza
This reverts commit 4a8c217.
I renamed some of the variables so that they are easier (for me anyway) to understand.

Also removed the `if (anyTrue and anyFalse)` as that condition will actually never be hit as specifying --no-flag and --flag <--- the latter flag will take precedence.

It's easier to just remove the check completely but if you guys think it's worth doing a check for contradictory flags then we can do that too but it could get complicated.

Otherwise the default behaviour will mean that the 'latest' flag gets set and I don't mind that.

Thoughts @milangfx @influenza

Co-Authored-By: milangfx <[email protected]>
Co-Authored-By: Ron Dahlgren <[email protected]>
1. Renamed `is_rate_limited` to `sleep_if_needed` to better clarify
behavior
2. Extract rate limiting error to a module constant
3. Add test suite and required dependencies
    Run 'nosetests' from your virtual environment to see it in action
4. Wrapped long lines
@ghost
Copy link
Author

ghost commented Jan 10, 2019

I looked at the changes and think it is good for now. I would wait to move the option parsing into its own function until some additional changes are made.

I would like to handle the flags differently so that we don't have to maintain two lists - one being the parameter provided to add_argument and the other being the strings in the list of tuples used to check for the flags. I'm thinking it might make sense to build a little object to hold this information.

On the other hand, it seems like a common problem so I wonder if there's some better way of using the argparse module that I'm missing 😆

@0xmilan
Copy link
Contributor

0xmilan commented Jan 10, 2019

I would leave the if (anyTrue and anyFalse) check so people at least get notified when they try to do --user-list --no-s3-scan --link-scan, and to avoid unexpected behaviour.

@0xmilan
Copy link
Contributor

0xmilan commented Jan 10, 2019

https://stackoverflow.com/questions/15008758/parsing-boolean-values-with-argparse
This is the best thing I found for the parameter handling. Also, there's parser.add_mutually_exclusive_group(required=False) which should protect against command --feature --no-feature, if we want that.

But this doesn't solve the problem of having to maintain two lists, and no doubt there's a better way of doing that than what I put together quickly.

@emtunc
Copy link
Owner

emtunc commented Jan 11, 2019

Hello @influenza ! Thank you for the PR - wondering if it would be a good idea to separate out testing to a separate branch? My knowledge is very limited in this area but it feels odd introducing dependencies (in requirements.txt) that won't be used by most/any of the users of the tool?

Are you able to join the public Slack Workspace I set up? Not an issue if you can't but wanted to discuss some testing integrations I was playing with earlier today.

@ghost
Copy link
Author

ghost commented Jan 11, 2019 via email

This reverts commit cf29e3a.
@emtunc emtunc merged commit 5ab0688 into emtunc:master Jan 11, 2019
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.

2 participants