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

adding warning when timeout is greater than 300 #1005

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

doron2402
Copy link

Description

GitHub Issues

@coveralls
Copy link

coveralls commented Jul 11, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling 29730c0 on doron2402:master into fa0e3cd on Miserlou:master.

@Miserlou
Copy link
Owner

This is a good thought, but since we'll need this for pretty much all of the commands and not just update, I'd say put this into load_settings directly when the setting is loaded rather than having a single use function that'd have to be added to all the primary command calls.

@doron2402
Copy link
Author

agree @Miserlou
I'll work on a better pr.

Thanks for the quick response

@doron2402
Copy link
Author

doron2402 commented Jul 12, 2017

  • validate timeout_seconds (min 1 max 300)
  • valid log_level

@Miserlou should we validate all the settings? seems too much, but definitely going to help debug your deployment script?

@Miserlou
Copy link
Owner

We should be doing more validation where we can, but the errors that cascade are generally self-explanatory. The ones like: #78 are more tricker and more time consuming but I'd really like to be able to handle those.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling d933d3f on doron2402:master into fa0e3cd on Miserlou:master.

@doron2402
Copy link
Author

yeah I was thinking about it making sure the file exists and the handler

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling 8a73367 on doron2402:master into fa0e3cd on Miserlou:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling 8a73367 on doron2402:master into fa0e3cd on Miserlou:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling 8a73367 on doron2402:master into fa0e3cd on Miserlou:master.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling 8a73367 on doron2402:master into fa0e3cd on Miserlou:master.

moving validation to load_settings

validate timeout_seconds

make sure module exists
@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling b06b6df on doron2402:master into fa0e3cd on Miserlou:master.

@coveralls
Copy link

coveralls commented Jul 12, 2017

Coverage Status

Coverage decreased (-60.2%) to 13.239% when pulling b06b6df on doron2402:master into fa0e3cd on Miserlou:master.

@doron2402
Copy link
Author

@Miserlou Should I validate all the settings? if so, how about breaking into a separate file that validates the settings?

@Miserlou
Copy link
Owner

Validate as much as you can, I suppose, but I don't think it needs to be a separate file, it can just be a function called from load_settings.

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