Skip to content

Use --no-random-sleep-on-renew during check#71

Merged
javierbertoli merged 1 commit intosaltstack-formulas:masterfrom
anderbubble:no-random-sleep-on-renew
Jul 7, 2020
Merged

Use --no-random-sleep-on-renew during check#71
javierbertoli merged 1 commit intosaltstack-formulas:masterfrom
anderbubble:no-random-sleep-on-renew

Conversation

@anderbubble
Copy link

@anderbubble anderbubble commented Jun 30, 2020

letsencrypt is consistently the longest state in my configuration, and apparently because of the random delay introduced when renewing non-interactively. Conceivably this could be made optional using a pillar variable; but, for now, I'll just disable the random sleep, at least during dry-run.

That said, I am left wondering whether check_letsencrypt_cert.sh and renew_letsencrypt_cert.sh might should just be the commands used even when using packages. It seems, for example, that check_letsencrypt_cert.sh checks the installed certs statically, without interaction with an external service, which seems preferable.

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [build] Changes related to the build system
  • [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
  • [ci] Changes to the continuous integration configuration
  • [feat] A new feature
  • [fix] A bug fix
  • [perf] A code change that improves performance
  • [refactor] A code change that neither fixes a bug nor adds a feature
  • [revert] A change used to revert a previous commit
  • [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

n/a

Describe the changes you're proposing

Use --no-random-sleep-on-renew during check_cert_cmd to minimize the time the state takes to run.

Pillar / config required to test the proposed changes

any valid effective configuration

Debug log showing how the proposed changes work

The random sleep can be seen in /var/log/letsencrypt/letsencrypt.log (or similar).

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@pull-assistant
Copy link

pull-assistant bot commented Jun 30, 2020

Score: 1.00

Best reviewed: commit by commit


Optimal code review plan

     fix(domains): renew --no-random-sleep-on-renew

Powered by Pull Assistant. Last update 07cbc3b ... 07cbc3b. Read the comment docs.

@javierbertoli
Copy link
Member

@anderbubble, thanks for this PR. Mind you amend your commit message so it passes the lint checks and follows the guidelines for commit messages? I'll merge this as soon as it passes the checks.

Regarding your other comment about the check scripts: I agree with you, some parts of this formula would benefit from refactoring to improve its behavior.

Any contribution will be welcome!

@myii
Copy link

myii commented Jul 3, 2020

@anderbubble To pass the commitlint, this will probably do:

fix(domains): use `--no-random-sleep-on-renew` during check

letsencrypt is consistently the longest state in my configuration, and
apparently because of the random delay introduced when renewing
non-interactively. Conceivably this could be made optional using a pillar
variable; but, for now, I'll just disable the random sleep, at least during
dry-run.

@javierbertoli If you want, we can just do this directly via. the Squash and merge feature in the GitHub UI.

@anderbubble
Copy link
Author

I'll tend to it right now.

letsencrypt is consistently the longest state in my configuration, and
apparently because of the random delay introduced when renewing
non-interactively. Conceivably this could be made optional using a
pillar variable; but, for now, I'll just disable the random sleep, at
least during dry-run.
@anderbubble anderbubble force-pushed the no-random-sleep-on-renew branch from 1efc4af to 07cbc3b Compare July 7, 2020 03:40
Copy link
Member

@javierbertoli javierbertoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, ready to merge.

@javierbertoli javierbertoli merged commit b0d89b3 into saltstack-formulas:master Jul 7, 2020
@javierbertoli
Copy link
Member

Thanks, @anderbubble!

@saltstack-formulas-travis

🎉 This PR is included in version 0.20.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants