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

Tag to disable random sleep upon a renew task #6599

Merged
merged 10 commits into from
Dec 19, 2018

Conversation

adferrand
Copy link
Collaborator

Following #6596, I extracted out my existing solution from #6541.

It adds a flag to the CLI, --no-random-sleep-on-renew to disable the random sleep when renew action is called from non interactive shells. The default behavior stays to make the random sleep if the flag is not provided.

The random sleep logic is also moved on layer down, to avoid to trigger it when no renew is going to be executed. Of course, this logic will be executed only once for a set of certificates to be renewed.

@bmw bmw self-assigned this Dec 13, 2018
@bmw bmw assigned ohemorange and unassigned bmw Dec 14, 2018
@adferrand
Copy link
Collaborator Author

Yes, I was charmed by Pycharm... and put more code cleaning that code logic. I will remove that, and make some PRs with only code cleaning, no changes of logic.

@adferrand
Copy link
Collaborator Author

So currently the logic in this PR is in sync with the latest comment of @bmw in #6596.

However there is still an open discussion about this. Here as the options so far:

  • visible flag
  • invisible flag
  • environment variable

I also expressed another proposition on IRC.

Because personally, I am more concerned about the hard coded time range than about enabling or not the sleep. Indeed I think now that the sleep should not be disabled, it is a good thing to dispatch load on ACME CA. However, I think that having a fixed time range, that will block a process for at most this time range "for no reason", and out of the context, is a weird approach.

What about the time range value, 8 min? Why not 10 min, 1 h or even 24 h to be sure to dispatch other the day?

So I would prefer to allow to modify the value of the range. Preferably as an hidden flag, as it is still more an option for certbot developers and integrators.

@adferrand
Copy link
Collaborator Author

All right @ohemorange. All changes are implemented, ready for new review.

@ohemorange
Copy link
Contributor

Thanks for making the changes!

Also, ooh, a shiny new tool that looks like it automatically PEP8ifies things...... maybe I'll try pycharm...

As for time range vs disable:

The two uses of this flag that are the reason we're putting it in are:

  1. for our packagers
  2. for our tests

For both of these, completely disabling it solves the problem. So in the spirit of YAGNI, I'm in favor of keeping it as is.

@adferrand
Copy link
Collaborator Author

FYI, after trying sublime text, atom, eclipse, visual studio code, I found out PyCharm to be the most advanced and efficient IDE I ever used for Python. But it makes non PEP8 compliant code extremely etchy ^^

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.

4 participants