-
Notifications
You must be signed in to change notification settings - Fork 479
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
Change the way we get environment variables #280
Conversation
detection_rules/misc.py
Outdated
elif prompt: | ||
return click.prompt(key, default=param.default if not param.default else None, hide_input=param.hide_input, | ||
show_default=True if param.default else False) | ||
def getenv(name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like what this is intended to solve, but this totally eliminates the ability to use config files for anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove all IO from the callback and then leave in the option to parse the value from a source based on precedence, which would still check for explicit, then envvar, then config option. Prompts can be moved to within their respective functions (as you did). Config parsing can be moved to the root group, where users can implicitly opt in to config parsing by storing in the default config file or optionally pass Thoughts? |
def kibana_upload(toml_files, kibana_url, cloud_id, user, password): | ||
"""Upload a list of rule .toml files to Kibana.""" | ||
from uuid import uuid4 | ||
from .packaging import manage_versions | ||
from .schemas import downgrade | ||
|
||
if not cloud_id or kibana_url: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would need to add a similar blurb for collect-events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note, then LGTM
I would also verify that the CLI.md doesn't need any updating as a result of this
Issues
Loosely related/discovered to this comment #17 (comment)
Summary
Got rid of click callback to use a non-interactive default variable. I think we were trying to do too much in a callback, and I believe any I/O in callback is discouraged.