-
Notifications
You must be signed in to change notification settings - Fork 9
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 *_config timeout to be specified as an env var/args. #26
Conversation
Hi @yjszk thank you for the update! And apologies, I miscommunicated something in my last email. I was hoping that we could set it up to work from either an env var OR from the passed in argument as you had written it before. Some users like to configure everything from env vars, but many are just running the commands a couple of times locally and don't want the extra overhead. Possible to support both? |
25bc749
to
fe1bf15
Compare
@aflanagan |
update readme on timeout issues Use CRONITOR_TIMEOUT env var, not arg. Support both args and env vars. typo fix env var is returned as stg type, it is int type. Remove code duplication revert CRONITOR_TIMEOUT logic celerybeat_only var delete ,miss fix add cronitor.timeout,test code fix
b2e3c90
to
3aa6092
Compare
@@ -17,6 +17,13 @@ | |||
api_version = os.getenv('CRONITOR_API_VERSION', None) | |||
environment = os.getenv('CRONITOR_ENVIRONMENT', None) | |||
config = os.getenv('CRONITOR_CONFIG', None) | |||
timeout = os.getenv('CRONITOR_TIMEOUT', None) |
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.
This is great!
One simplifying change I would like to consider (and am open to feedback on), is to simply set
timeout
here, and then read that internally in the _put
and as_yaml
methods instead of passing timeout as an arg to the various *_config
methods.
I know this isn't quite what I had suggest before, but it would simplify the code and the interface a little bit.
import cronitor
cronitor.timeout = 30
cronitor.apply_config()
internally the timeout attribute would just be set as, but could still be set directly on the module as done in the above example.
timeout = os.getenv('CRONITOR_TIMEOUT', 10)
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.
Thanks for the review! i have made the code shorter and easier to read.
And README has also been updated. Please check!
431222c
to
a452913
Compare
@aflanagan @shaneharter
If the number of monitors exceeds 150, the following error occurs in validate_config and apply_config.
The timeout value is fixed to 10 seconds inside the library and we wanted to be able to set it with env var/args.