Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

maxChecks cli arg --> ACME_AUTH_STATUS_MAX_CHECKS #166

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

alanbacon
Copy link
Contributor

Thank you for contributing to sewer.
Every contribution to sewer is important to us.
You may not know it, but you have just contributed to making the world a more safer and secure place.

Contributor offers to license certain software (a “Contribution” or multiple
“Contributions”) to sewer, and sewer agrees to accept said Contributions,
under the terms of the MIT License.
Contributor understands and agrees that sewer shall have the irrevocable and perpetual right to make
and distribute copies of any Contribution, as well as to create and distribute collective works and
derivative works of any Contribution, under the MIT License.

Now,

What(What have you changed?)

added a cli flag to set the number of attempts sewer makes to verfify the DNS challenge

Why(Why did you change it?)

Because sewer was timing out when trying to verify the DNS challenge, and so I needed it to wait for longer

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #166 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #166   +/-   ##
=======================================
  Coverage   86.71%   86.71%           
=======================================
  Files          18       18           
  Lines        1024     1024           
=======================================
  Hits          888      888           
  Misses        136      136           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3eee58...3c0839f. Read the comment docs.

@alanbacon
Copy link
Contributor Author

@komuw I managed to format this PR correcting using black, would you mind helping me with the formatting on my other PR thanks

@mmaney
Copy link
Collaborator

mmaney commented Apr 14, 2020

@alanbacon wouldn't it be better, at the CLI level, to request the total time to wait before giving up? Yes, I know that's not how Client expresses it now... that might be a related (or separate?) change. Maybe just define the defaults for request_timeout and max_tries in the client module so that cli can query them before initializing its Client instance.

I assume you've used your patches - what sort of total timeout did you need? Just looking for a datapoint here. Thanks.

@alanbacon
Copy link
Contributor Author

@mmaney A total timeout flag would be much better you're right, I just quickly hashed this together and thought that the project might want the change also. I used 10 x 8s ~= 1m:30s. The current default of 3 x 8s 24s is a little on the tight side for me, maybe my provider (gandi) just isn't all that great I don't know. 24s is sort of number that i feel will work maybe 90% of the time.

@mmaney
Copy link
Collaborator

mmaney commented Apr 14, 2020

@alanbacon Okay... it's been a long day, I need to sleep on this. We've had a couple of mentions of timeout issues, and that wait-for-all-validations loop is rather a mess in other ways. Thanks to this discussion, I'm thinking it should be looping not until N queries have been made (and there was a bug until recently where the last try was never accepted), but until the seperate VALIDATION_TIMEOUT period of time has passed. This is a deeper refactoring of get_certificate and check_authorization_status that I've had somewhere down my ToDo list for a while. I think you've shown me why I wasn't entirely happy with the cleanup of the counting loop I'd had in mind.

@mmaney mmaney added this to the 0.9 milestone Jun 30, 2020
@mmaney mmaney modified the milestones: 0.9, 0.8.5 Sep 10, 2020
@mmaney mmaney self-assigned this Sep 10, 2020
@mmaney mmaney added the refactoring a sizable chunk of code needs to change label Sep 10, 2020
@mmaney
Copy link
Collaborator

mmaney commented Sep 10, 2020

With the get_certificate mess targeted for 0.8.5, I expect fixing this (by adding a simple timeout instead of MAX_COUNT) will be a side effect. The rest of that work will be the challenge. ;-/

@mmaney mmaney modified the milestones: 0.8.5, 0.9 Nov 17, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
refactoring a sizable chunk of code needs to change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants