Created attachment 161275 [details] patch This patch resolves FreeBSD issue #203047 and upstream SaltStack #27151. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203047 https://github.com/saltstack/salt/issues/27151
Created attachment 161276 [details] QA
We should also probably add something to UPDATING to outline the PATH requirements as outlined in the pkg-message for this release.
salt-proxy.in: So there's a few things to point out that I've updated. We have to use the substitution syntax here like so... command="%%PREFIX%%/bin/salt-proxy" command_interpreter="%%PYTHON_CMD%%" required_files="%%PREFIX%%/etc/salt" This was missing the $ for the variable and we can handle this better [ -n "salt_proxy_list" ] || return How about? if [ -n "${salt_proxy_list}" ]; then echo "${salt_proxy_list} is undefined" return 1 fi Questions 1. What's the PYTHON_EGG_CACHE for? pkg-message.in: Shouldn't the sysrc salt_minion_paths be under Salt proxy minion as well? I added it since we are dealing with the PATH in there.
Regarding UPDATING and PATH. Would it be better to define it as a default? PHB syntax regarding optional elements and defaults may be pointing out if you haven't already read it. https://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/rc-scripts.html For optional configuration elements the "=" style of default variable assignment is preferable to the ":=" style here, since the former sets a default value only if the variable is unset, and the latter sets one if the variable is unset or null. A user might very well include something like:
Created attachment 161407 [details] py27-salt-2015.8.0_1.patch My version of the patch. I've also taken the liberty fix some noise regarding quoting that devel/rclint warns about as well as fix the $FreeBSD$ tag per the PHB link I mentioned above. I would appreciate clarification on the questions above and a thumbs up that everything looks good.
This as well from PHB for the ${salt_proxy_list}. I need to fix this in my patch. Are there default assignments to empty strings? They should be removed, but double-check that the option is documented in the comments at the top of the file.
(In reply to Jason Unovitch from comment #3) 1) My mistake about the %%PREFIX%%; thank you for catching that. 2) The PYTHON_EGG_CACHE was added at some point in the past and is a component that I don't fully understand. If it's no longer needed, let's remove it. (In reply to Jason Unovitch from comment #4) 1) I would prefer to set the salt_{proxy,minion,master}_paths as default values instead of in the pkg-message.in or in UPDATING. In the current revision it uses $PATH which, for whatever reason, does not include %%PREFIX%%/{bin,sbin} but needs to. Thank you for the help with this. Please let me know if there is anything more that I can clarify.
Created attachment 161452 [details] py27-salt-2015.8.0_1.patch (In reply to Christer Edwards from comment #7) Regarding PYTHON_EGG_CACHE... I looks like that was added in a prior revision for diskless systems. https://svnweb.freebsd.org/ports?view=revision&revision=299831 Regarding PATH, this revision sets a sane default value with %%PREFIX%% so that no user interaction will be needed on upgrade. The pkg-message no longer mentions setting these. I also made the rc script headers a bit more verbose to match the style of other ports scripts. So the headers mention the reason for the EGG_CACHE for example is for diskless systems. Does this work for you?
Commit for PATH and PYTHON_EGG_CACHE: https://svnweb.FreeBSD.org/ports?view=revision&revision=299831
Yes, this looks good. Thanks for your help with improving this.
A commit references this bug: Author: junovitch Date: Sun Sep 27 14:32:07 UTC 2015 New revision: 398042 URL: https://svnweb.freebsd.org/changeset/ports/398042 Log: sysutils/py-salt: add new rc scripts, clean up current scripts - Add salt-proxy.in and associated pkg-message text for Salt Proxy - Restore installation of salt-api.in accidentally removed in r396791 - Set a default for PATH to address upstream issue 27151 - While here, make headers more verbose and trim variable quotes per rclint PR: 203262 Submitted by: Christer Edwards <christer.edwards@gmail.com> (maintainer) Changes: head/sysutils/py-salt/Makefile head/sysutils/py-salt/files/pkg-message.in head/sysutils/py-salt/files/salt_api.in head/sysutils/py-salt/files/salt_master.in head/sysutils/py-salt/files/salt_minion.in head/sysutils/py-salt/files/salt_proxy.in head/sysutils/py-salt/files/salt_syndic.in
Committed. Thank you Christer!