Bug 203262

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.

Comment 3 Jason Unovitch 2015-09-26 02:04:04 UTC

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.

Comment 4 Jason Unovitch 2015-09-26 02:07:32 UTC

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:

Comment 5 Jason Unovitch 2015-09-26 02:12:26 UTC

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.

Comment 6 Jason Unovitch 2015-09-26 02:26:27 UTC

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.

Comment 8 Jason Unovitch 2015-09-27 02:57:16 UTC

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?

Comment 9 Jason Unovitch 2015-09-27 02:58:23 UTC

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.

Comment 11 commit-hook 2015-09-27 14:32:48 UTC

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

Comment 12 Jason Unovitch 2015-09-27 14:36:57 UTC

Committed.  Thank you Christer!