Add environment parameter to renew cron #288
Conversation
gmenuel
force-pushed
the
add-environment-cron
branch
3 times, most recently
from
de10e62
to
6da637d
Compare
|
Thanks! |
manifests/renew.pp
Outdated
| Letsencrypt::Cron::Hour $cron_hour = $letsencrypt::renew_cron_hour, | ||
| Letsencrypt::Cron::Minute $cron_minute = $letsencrypt::renew_cron_minute, | ||
| Letsencrypt::Cron::Monthday $cron_monthday = $letsencrypt::renew_cron_monthday, | ||
| Variant[String, Array[String], Undef] $cron_environment = $letsencrypt::renew_cron_environment, |
There was a problem hiding this comment.
Would it make sense for the type to default to an empty array and disallow Undef? I'm not sure if that makes a difference for the cron type.
There was a problem hiding this comment.
I've tested with [] and it removed the environment if it was previously setted. With the option unset (or undef) it doesn't remove the environment. But I think that removing the environment might be the right thing to do.
There was a problem hiding this comment.
I've tested with [] and it removed the environment if it was previously setted. With the option unset (or undef) it doesn't remove the environment. But I think that removing the environment might be the right thing to do.
@gmenuel I wonder then if we should mark this as a breaking change, since it would remove manual changes to the cron job.
There was a problem hiding this comment.
@kenyon yes you are right, it is probably better to tag this as a breaking change.
gmenuel
force-pushed
the
add-environment-cron
branch
from
6da637d
to
edefa32
Compare
|
Just a quick question, is there anything holding this PR back? |
gmenuel
force-pushed
the
add-environment-cron
branch
from
5a9b830
to
9eddbb1
Compare
|
Merge conflicts resolved. |
Pull Request (PR) description
This PR add an option for adding environment to the global renew cron, this is useful for example with the nginx certbot plugin which somehow can't find nginx if the PATH in the cron doesn't include /usr/sbin.
This PR is similar to #189 which doesn't seem really active.
This Pull Request (PR) fixes the following issues
Fixes: #250 (well kind of, it doesn't directly fix this, but it allows to add the PATH in the cron which will fix the problem).