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

Add cron time parameters #122

Merged
merged 1 commit into from
Dec 11, 2018
Merged

Add cron time parameters #122

merged 1 commit into from
Dec 11, 2018

Conversation

matonb
Copy link
Contributor

@matonb matonb commented Jun 2, 2018

Accepts puppet cron type time attributes

Pull Request (PR) description

Enable user to define cron time attributes instead of defaulting to random time

This Pull Request (PR) fixes the following issues

#47 It doesn't fix this issue per-se but does allow users to specify more than one execution time

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Makes sense.

manifests/certonly.pp Outdated Show resolved Hide resolved
$cron_before_command = undef,
$cron_success_command = undef,
# 0 - 23, seed is title plus fqdn
Variant[Integer, String, Array] $cron_hour = fqdn_rand(24, $title),
Copy link
Member

Choose a reason for hiding this comment

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

You can limit it further by specifying Integer[0, 23]. At least, I think that was inclusive. Also wondering if the Array should be further specified. A similar comment applies to the minute parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add the limit to the integer field, the array I'm not so sure about...

@bastelfreak
Copy link
Member

Hi @matonb, could you rebase please?

@matonb
Copy link
Contributor Author

matonb commented Oct 15, 2018

@bastelfreak rebased

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks like it needs a new rebase :(

manifests/certonly.pp Outdated Show resolved Hide resolved
@alexjfisher
Copy link
Member

@matonb Thanks for the squash/rebase. Would you like to amend the final commit message though?

@alexjfisher alexjfisher added enhancement New feature or request and removed needs-rebase labels Dec 5, 2018
ekohl
ekohl previously requested changes Dec 5, 2018
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Small nit, 👍 otherwise

README.md Outdated
You can optionally add a shell command to be run on success using the `cron_success_command` parameter.
You can optionally add a shell command to be run on before using the `cron_before_command` parameter.
You can optionally specify one or multiple days of the month when to run the cron using the `cron_monthday` parameter (default is every day).
To automatically renew a certificate, you can pass the `manage_cron` parameter.
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the trailing whitespace here?

Copy link
Contributor Author

@matonb matonb Dec 5, 2018

Choose a reason for hiding this comment

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

@ekohl Added for newlines when renderd in markdown, is that a problem?

Copy link
Member

Choose a reason for hiding this comment

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

A lot of scripts and editors automatically strip trailing whitespace and it's also very hard to review. It's probably better to rewrite as a list:

  • manage_cron can be used to automatically renew the certificate
  • cron_success_command can be used to run a shell command on a successful renewal
  • cron_before_command can be used to run a shell command before a renewal
  • ....

@alexjfisher alexjfisher dismissed ekohl’s stale review December 11, 2018 22:42

I've updated the README in the way suggested.

@alexjfisher alexjfisher merged commit 63fd8aa into voxpupuli:master Dec 11, 2018
@alexjfisher
Copy link
Member

@matonb Thanks! Sorry for the delay in getting this in.

@Dan33l
Copy link
Member

Dan33l commented Jan 14, 2019

@matonb Thank you.
Your PR is now release on the puppet forge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants