Merge lp:~jamesodhunt/upstart/initctl2dot-fix-for-subdirs into lp:upstart

[James Hunt]

* scripts/initctl2dot.py:
  - footer(): Add details of session.
  - sanitise(): Handle jobs in sub-directories.
  - main(): Add --user and --system options and determine correct
    session to connect to.
* scripts/man/initctl2dot.8:
  - Added --user and --system options.
  - Escape dashes in options.
  - Update date.

[Dimitri John Ledkov (xnox)]

On 22 March 2013 12:37, James Hunt <email address hidden> wrote:
> # Map dash to underscore since graphviz node names cannot
> -# contain dashes. Also remove dollars and colons
> +# contain dashes. Also remove dollars and colons and replace other
> +# punctuation with graphiviz-safe names.
> def sanitise(s):
> return s.replace('-', '_').replace('$', 'dollar_') \
> .replace('[', 'lbracket').replace(']', 'rbracket') \
> .replace('!', 'bang').replace(':', 'colon').replace('*', 'star') \
> - .replace('?', 'question').replace('.', '_')
> + .replace('?', 'question').replace('.', '_').replace('/', '_')
>

I wonder if we can use translate instead here:
sanitize_table = str.maketrans({
    '-': '_',
    '$': 'dollar_',
    '[': 'lbracker',
    ']': 'rbracker',
    '!': 'bang',
    ':': 'colon',
    '*': 'star',
    '?': 'question',
    '.': '_',
    '/': '_',
})
return s.translate(sanitize_table)

Somehow this is easier to read & edit =) maybe make that translation
table global, such that we don't create that object each time
sanitized is called.

>
> + parser.add_argument("--user",
> + dest="user",
> + action='store_true',
> + help="Connect to Upstart user session (default if running within a user session).")
> +
> + parser.add_argument("--system",
> + dest="system",
> + action='store_true',
> + help="Connect to Upstart system session.")
> +
> parser.set_defaults(color_emits=default_color_emits,
> color_start_on=default_color_start_on,
> color_stop_on=default_color_stop_on,
> @@ -504,6 +535,25 @@
> if options.restrictions:
> restrictions_list = options.restrictions.split(",")
>
> + upstart_session = os.environ.get('UPSTART_SESSION')
> +

I'd change it to be os.environ.get('UPSTART_SESSION', False), such
that we get explicit False, instead of vague None when it's not
defined.

> + use_system = True
> +
> + if options.system == False and options.user == False:
> + if upstart_session:
> + use_system = False
> + else:
> + use_system = True
> + elif options.system == True:
> + use_system = True
> + elif options.user == True:
> + use_system = False
> +

"if foo == True" is not pythonic, simply use "if foo:"

Further more you are using if comparison to derive and assign a bollean value.
Why not just assign it?

use_system = options.system or not options.user or not upstart_session

Or you can make it easier by defining the option --user to be
dest='use_system', action='store_false' & --system to be
dest='use_system', action='store_true' then you will only need to test
for the environment variable.
(It will give less flexibility in the future, but I don't think these
two options will evolve beyond simple binary choice).

Regards,

Dmitrijs.

[Dimitri John Ledkov (xnox)]

review: Needs Fixing

[lp:~jamesodhunt/upstart/initctl2dot-fix-for-subdirs]

1459. By James Hunt on 2013-03-22

* scripts/initctl2dot.py:
  - sanitise(): Use global translation table.
  - main(): Remove need for options.user.

1460. By James Hunt on 2013-03-22

* scripts/initctl2dot.py: footer(): Make more pythonic.

1461. By James Hunt on 2013-03-22

* scripts/initctl2dot.py: sanitise(): Rather important typo fix.

1462. By James Hunt on 2013-03-22

* scripts/initctl2dot.py: Remove cruft.

[James Hunt (jamesodhunt)]

Hi Dmitrijs,

Thanks - I've added the translation table and default value for UPSTART_SESSION.

Regarding the option handling, I had started out as you suggest, but ArgumentParser is slightly odd wrt args that use a shared variable. I have now improved the code by adding a default value and following your suggestions as best that allows.

[Dimitri John Ledkov (xnox)]

Fair enough. We can iterate on it later.

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2013-03-22 10:55:18 +0000
+++ ChangeLog	2013-03-22 13:57:01 +0000
@@ -1,3 +1,15 @@
[phone]  James Hunt  <[e-mail]>
+
+	* scripts/initctl2dot.py:
+	  - footer(): Add details of session.
+	  - sanitise(): Handle jobs in sub-directories.
+	  - main(): Add --user and --system options and determine
+	    correct session to connect to.
+	* scripts/man/initctl2dot.8:
+	  - Added --user and --system options.
+	  - Escape dashes in options.
+	  - Update date.
+
 2013-03-21  James Hunt  <[e-mail]>
 
 	* po/POTFILES.in:

=== modified file 'NEWS'
--- NEWS	2013-03-04 11:57:18 +0000
+++ NEWS	2013-03-22 13:57:01 +0000
@@ -1,4 +1,4 @@
-1.8  xxxx-xx-xx
+1.8  2013-03-22 "Scone + Radish = Gherkin?"
 
 1.7  2013-03-04 "We've all got one now"
 

=== modified file 'scripts/initctl2dot.py' (properties changed: -x to +x)
--- scripts/initctl2dot.py	2012-12-07 18:26:43 +0000
+++ scripts/initctl2dot.py	2013-03-22 13:57:01 +0000
@@ -57,12 +57,28 @@
 options = None
 jobs = {}
 events = {}
-cmd = "initctl --system show-config -e"
 script_name = os.path.basename(sys.argv[0])
 
+cmd = None
+use_system = True
+upstart_session = None
+
 # list of jobs to restict output to
 restrictions_list = []
 
+sanitise_table = str.maketrans({
+    '-': '_',
+    '$': 'dollar_',
+    '[': 'lbracker',
+    ']': 'rbracker',
+    '!': 'bang',
+    ':': 'colon',
+    '*': 'star',
+    '?': 'question',
+    '.': 'dot',
+    '/': 'slash',
+})
+
 default_color_emits = 'green'
 default_color_start_on = 'blue'
 default_color_stop_on = 'red'
@@ -86,10 +102,20 @@
 
 
 def footer(ofh):
+    global upstart_session
+    global use_system
+
+    details = ''
+
+    if use_system:
+        details += "\\nfor the system\\n"
+    else:
+        details += "\\nfor session '%s'\\n" % upstart_session
+
     if options.restrictions:
-        details = "(subset, "
+        details += "(subset, "
     else:
-        details = "("
+        details += "("
 
     if options.infile:
         details += "from file data)."
@@ -109,13 +135,12 @@
                             script_name=script_name, details=details))
 
 
-# Map dash to underscore since graphviz node names cannot
-# contain dashes. Also remove dollars and colons
+# Map punctuation to symbols palatable to graphviz
+# (which amongst other things dislikes dashes in node names)
 def sanitise(s):
-    return s.replace('-', '_').replace('$', 'dollar_') \
-            .replace('[', 'lbracket').replace(']', 'rbracket') \
-            .replace('!', 'bang').replace(':', 'colon').replace('*', 'star') \
-            .replace('?', 'question').replace('.', '_')
+    global sanitise_table
+
+    return s.translate(sanitise_table)
 
 
 # Convert a dollar in @name to a unique-ish new name, based on @job and
@@ -334,6 +359,10 @@
 
 
 def read_data():
+    global cmd
+    global upstart_session
+    global use_system
+
     if options.infile:
         try:
             ifh = open(options.infile, 'r')
@@ -412,6 +441,9 @@
 def main():
     global options
     global restrictions_list
+    global cmd
+    global use_system
+    global upstart_session
 
     description = "Convert initctl(8) output to GraphViz dot(1) format."
     epilog = "See http://www.graphviz.org/doc/info/colors.html " \
@@ -426,8 +458,8 @@
 
     parser.add_argument("-f", "--infile",
                         dest="infile",
-                        help="File to read '%s' output from. If not specified"
-                        ", initctl will be run automatically." % cmd)
+                        help="File to read output from. If not specified"
+                        ", initctl will be run automatically.")
 
     parser.add_argument("-o", "--outfile",
                         dest="outfile",
@@ -479,6 +511,18 @@
                         help="Specify color for job boxes (default=%s)." %
                              default_color_job)
 
+    parser.add_argument("--user",
+                        dest="system",
+                        default=None,
+                        action='store_false',
+                        help="Connect to Upstart user session (default if running within a user session).")
+
+    parser.add_argument("--system",
+                        dest="system",
+                        default=None,
+                        action='store_true',
+                        help="Connect to Upstart system session.")
+
     parser.set_defaults(color_emits=default_color_emits,
                         color_start_on=default_color_start_on,
                         color_stop_on=default_color_stop_on,
@@ -504,6 +548,21 @@
     if options.restrictions:
         restrictions_list = options.restrictions.split(",")
 
+    upstart_session = os.environ.get('UPSTART_SESSION', False)
+
+    if options.system == None:
+        if upstart_session:
+            use_system = False
+        else:
+            use_system = True
+    else:
+        use_system = options.system or not upstart_session
+
+    if use_system:
+        cmd = "initctl --system show-config -e"
+    else:
+        cmd = "initctl show-config -e"
+
     read_data()
 
     for job in restrictions_list:

=== modified file 'scripts/man/initctl2dot.8'
--- scripts/man/initctl2dot.8	2011-06-01 14:28:20 +0000
+++ scripts/man/initctl2dot.8	2013-03-22 13:57:01 +0000
@@ -1,4 +1,4 @@
-.TH initctl2dot 8 2011-03-07 "Upstart"
+.TH initctl2dot 8 2013-03-22 "Upstart"
 .\"
 .SH NAME
 initctl2dot \- manual page for initctl2dot
@@ -17,56 +17,64 @@
 With no options,
 .BR initctl (8)
 will be invoked automatically and the output written to file
-\fIupstart.dot\fP.
+\fIupstart.dot\fP. If run from within an Upstart user session, unless
+.B \-\-system
+is specified, the data generated will be for the user session.
 .\"
 .SH OPTIONS
 .TP
-.B -h
-Display usage statement.
-.TP
-\fB-f\fP \fIINFILE\fP , \fP--infile\fP=\fIINFILE\fP
+\fB\-f\fP \fIINFILE\fP , \fP\-\-infile\fP=\fIINFILE\fP
 File to read
 .BR initctl (8)
 output from ("initctl show-config -e"). If not specified,
 .BR initctl (8)
 will be run automatically.
 .TP
-\fB-w\fP \fIOUTFILE\fP , \fP--outfile\fP=\fIOUTFILE\fP
-File to write output to.
+.B \-h
+Display usage statement.
 .TP
-\fB-r\fP \fIRESTRICTIONS\fP , \fP--restrict-to-jobs\fP=\fIRESTRICTIONS\fP
+\fB\-r\fP \fIRESTRICTIONS\fP , \fP\-\-restrict-to-jobs\fP=\fIRESTRICTIONS\fP
 Limit display of
 .B start on
 and
 .B stop on
 conditions to comma-separated list of jobs.
 .TP
-\fB--color-emits\fP=\fICOLOR_EMITS\fP
+\fB\-w\fP \fIOUTFILE\fP , \fP\-\-outfile\fP=\fIOUTFILE\fP
+File to write output to.
+.TP
+\fB\-\-color-emits\fP=\fICOLOR_EMITS\fP
 Specify color for 'emits' lines.
 .TP
-\fB--color-start-on\fP=\fICOLOR_START_ON\fP
+\fB\-\-color-start-on\fP=\fICOLOR_START_ON\fP
 Specify color for 'start on' lines.
 .TP
-\fB--color-stop-on\fP=\fICOLOR_STOP_ON\fP
+\fB\-\-color-stop-on\fP=\fICOLOR_STOP_ON\fP
 Specify color for 'stop on' lines.
 .TP
-\fB--color-event\fP=\fICOLOR_EVENT\fP
+\fB\-\-color-event\fP=\fICOLOR_EVENT\fP
 Specify color for event boxes.
 .TP
-\fB--color-text\fP=\fICOLOR_TEXT\fP
+\fB\-\-color-text\fP=\fICOLOR_TEXT\fP
 Specify color for summary text.
 .TP
-\fB--color-bg\fP=\fICOLOR_BG\fP
+\fB\-\-color-bg\fP=\fICOLOR_BG\fP
 Specify background color for diagram.
 .TP
-\fB--color-event-text\fP=\fICOLOR_EVENT_TEXT\fP
+\fB\-\-color-event-text\fP=\fICOLOR_EVENT_TEXT\fP
 Specify color for text in event boxes.
 .TP
-\fB--color-job-text\fP=\fICOLOR_JOB_TEXT\fP
+\fB\-\-color-job-text\fP=\fICOLOR_JOB_TEXT\fP
 Specify color for text in job boxes.
 .TP
-\fB--color-job\fP=\fICOLOR_JOB\fP
+\fB\-\-color-job\fP=\fICOLOR_JOB\fP
 Specify color for job boxes.
+.TP
+\fB\-\-system\fP
+Connect to the Upstart system session.
+.TP
+\fB\-\-user\fP
+Connect to the Upstart user session.
 .\"
 .SH AUTHOR
 Written by James Hunt