Proposed commit log:
security/letsencrypt.sh: Update to 2016-02-12 - Update to 2016-02-16 - Add options for ZSH PR: 206976 Reviewed_by: feld (mentor), koobs (mentor), sascha (maintainer) Approved by: (mentor) Differential_Revision: D5264
Details
Details
- portlint -AC
- poudriere testport (OK)
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 2484 Build 2501: arc lint + arc unit
Event Timeline
Comment Actions
Mailed maintainer
On 2016-02-12 22:05, Bernard Spil wrote:
Hi Sascha,
I've created a review for security/letsencrypt.sh to work with ZSH in
addition to BASH. Parts of my original patch were included upstream.
Can you provide a maintainer-approval?
Thanks!
Bernard Spil
https://wiki.freebsd.org/BernardSpil/LetsEncrypt
Sorry... Forgot to paste a link to the Phabricator Review
https://reviews.freebsd.org/D5264Comment Actions
Update periodic job
- Allow running as a different user
- Allow post-script for deployment
Comment Actions
I've no objections to the proposed changes but I still have an update request open here:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206976
I'll close that in favour of this one but would ask you to incorporate two changes from the bug report:
- Add the hook.sh.example file to the install target and plist
- Add hook.sh.example and letsencrypts.sh itself to the SHEBANG_FILES list
The latter one might interfere with the zsh replacement as I haven't checked when exactly
SHEBANGFIX is run. This might need some testing.
pkg-message also needs an update to reflect the change from monthly_letsencrypt_enable to weekly_letsencrypt_enable.
Comment Actions
Sorry, hadn't noticed there was a PR already. Thanks for the additions, I think I've merged these in. All installed scripts have ZSH when that has been selected as an option.
Comment Actions
Thanks for the additions! Please commit.
I couldn't find a maintainer-approved flag as i've never used phabricator before. If i need to set one please let me know how.
Comment Actions
The call in the periodic script does nothing if the configured max age of the certificates isn't reached.
So the only thing this changes for existing users is that they can be more granular with their age settings.
One could make a case to do this daily as the age is configured in days but i think a week is good enough.
If not this can be fixed with another update if enough users need this.
The only thing that gets called every time after this patch will be the new weekly_letsencrypt_deployscript, if set.
But as this is introduced with this patch it doesn't affect existing users.
Comment Actions
Users still have to update their /etc/periodic.conf to say "monthly" instead of "weekly" or it will just stop running, but since this is such a new port with hardly any users, I think it should be fine.
Comment Actions
The periodic script is missing a "then" in the last if clause which results in a sytax error:
/usr/local/etc/periodic/weekly/000.letsencrypt.sh: 21: Syntax error: "fi" unexpected (expecting "then")
Fix:
- 000.letsencrypt.sh.orig 2016-02-14 23:45:40.527800680 +0100
+++ 000.letsencrypt.sh 2016-02-14 23:42:32.160814076 +0100
@@ -17,6 +17,7 @@
su -m "$weekly_letsencrypt_user" -c '/usr/local/bin/letsencrypt.sh -c' fi if [ -x "$weekly_letsencrypt_deployscript" ]
+ then
$weekly_letsencrypt_deployscript fi ;;