Bug 203262 - [MAINTAINER] sysutils/py-salt : resolve #203047 & upstream #27151
Summary: [MAINTAINER] sysutils/py-salt : resolve #203047 & upstream #27151
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Jason Unovitch
Keywords: feature, regression
Depends on:
Reported: 2015-09-22 15:13 UTC by Christer Edwards
Modified: 2015-09-27 14:36 UTC (History)
1 user (show)

See Also:

patch (3.59 KB, text/plain)
2015-09-22 15:13 UTC, Christer Edwards
no flags Details
QA (400.02 KB, text/plain)
2015-09-22 15:14 UTC, Christer Edwards
no flags Details
py27-salt-2015.8.0_1.patch (5.47 KB, patch)
2015-09-26 02:12 UTC, Jason Unovitch
no flags Details | Diff
py27-salt-2015.8.0_1.patch (6.95 KB, patch)
2015-09-27 02:57 UTC, Jason Unovitch
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christer Edwards 2015-09-22 15:13:47 UTC
Created attachment 161275 [details]

This patch resolves FreeBSD issue #203047 and upstream SaltStack #27151.

Comment 1 Christer Edwards 2015-09-22 15:14:18 UTC
Created attachment 161276 [details]
Comment 2 Christer Edwards 2015-09-22 22:42:33 UTC
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 freebsd_committer 2015-09-26 02:04:04 UTC

So there's a few things to point out that I've updated.  We have to use the substitution syntax here like so...


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

1. What's the PYTHON_EGG_CACHE for?

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 freebsd_committer 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.

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 freebsd_committer 2015-09-26 02:12:26 UTC
Created attachment 161407 [details]

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 freebsd_committer 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.
Comment 7 Christer Edwards 2015-09-26 23:45:49 UTC
(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 freebsd_committer 2015-09-27 02:57:16 UTC
Created attachment 161452 [details]

(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.


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 freebsd_committer 2015-09-27 02:58:23 UTC
Comment 10 Christer Edwards 2015-09-27 03:27:50 UTC
Yes, this looks good. Thanks for your help with improving this.
Comment 11 commit-hook freebsd_committer 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

  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)

Comment 12 Jason Unovitch freebsd_committer 2015-09-27 14:36:57 UTC
Committed.  Thank you Christer!