/juju

[jjo] juju2 bash autocompletion improvements #5057

Merged
merged 15 commits into from Jun 29, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@jjo @bz2 @mjs @cherylj @jujubot @jrwren
@jjo
Contributor

jjo commented Apr 8, 2016

  • use python3
  • --model support (including juju ssh --model ..., etc)
  • --controller support
  • more aggressive caching (mainly needed for controller*model completions)
etc/bash_completion.d/juju2
@@ -1,42 +1,55 @@
+#!/bin/bash
@bz2

bz2 Apr 9, 2016

Contributor

Lintian told someone to remove this from the file. I didn't see the message so don't know why, but not that:

grep -F '#!/' /usr/share/bash-completion/completions/*

Matches nothing on my system.

@jjo

jjo Jun 8, 2016

Contributor

Fixed.

etc/bash_completion.d/juju2
# juju-core.bash_completion.sh: dynamic bash completion for juju cmdline,
# from parsed (and cached) juju status output.
#
# Author: JuanJo Ciarlante <jjo@canonical.com>
# Copyright 2013+, Canonical Ltd.
# License: GPLv3
#
+# For juju2, includes --model and --controller handling:
@bz2

bz2 Apr 9, 2016

Contributor

Per the recent mail to #juju-dev, the binary name will just be 'juju'. I was looking at this script for the packaging, and think what I need is a top level variable JUJU=juju that I can replace when installing, so the wrapping scripts 'juju-1' and 'juju-2.0' can also get correct completetion goodness.

@jjo

jjo Jun 8, 2016 •
Edited 1 time

Contributor

Fixed with a run-time approach, see comments in added "juju-version" file.
Note that being run-time, supports whatever "juju" [verbatim] cmd version the user currently have, even if e.g. aliased.

etc/bash_completion.d/juju2
-_juju_machines_from_file() {
-python -c '
+_juju_machines_from_status_file() {
+python3 -c '
@bz2

bz2 Apr 9, 2016

Contributor

We want this script to work on backports to trusty etc, which may have python3 available, but not installed. Can we also have a $PYTHON variable that gets set the appropriate default for the system?

@jjo

jjo Jun 8, 2016

Contributor

Fixed, added _juju_cmd_PYTHON env var.

etc/bash_completion.d/juju2
+_juju_list_controllers_noflags() {
+ _juju_cache_cmd 10 cat \
+ juju list-controllers --format json | \
+ python3 -c 'import json,sys; print("\n".join(json.load(sys.stdin)["controllers"].keys()))'
@bz2

bz2 Apr 9, 2016

Contributor

Will need a space after print before ( for this to work on Python 2 and 3.

Could make the json parsing helper take a 'key' name to work for all these cases? Just sub in machines/services/controllers?

@jjo

jjo Jun 8, 2016 •
Edited 1 time

Contributor

Thanks the spacing for python2,3.
Regarding generalizing the functions to receive a key instead, I'd rather prefer to keep them explicit to avoid adding another "layer of indirection", for readability and debugging.

etc/bash_completion.d/juju2
return $?
}
complete -F _juju juju
+complete -F _juju juju2
@bz2

bz2 Apr 9, 2016

Contributor

Same note about command name still being 'juju'.

@bz2
Contributor

bz2 commented Apr 9, 2016

Thanks!

Have added various comments inline, main note being about the binary name.
etc/bash_completion.d/juju
+ juju help commands 2>/dev/null | awk '{print $1}'
+}
+
+# Print (return) flags for juju action, shamelessly excluding
@jrwren

jrwren Jun 7, 2016

Member

The comment re: shamelessly excluding -m/--model is no longer true.

@jjo

jjo Jun 7, 2016

Contributor

Thanks, fixed.

@jjo
Contributor

jjo commented Jun 8, 2016

I've addressed bz2 comments (tho in a different way than proposed), PTAL.
@jjo
Contributor

jjo commented Jun 17, 2016 •
Edited 1 time

Appreciate another review - several improvements since last PR:

  • add python2,3 loadtime detection
  • 1.0 and 2.0 version handling:
    • rename completion file to juju-2.0
    • add juju-version file for run-time 1.x/2.0 completion
      (as juju-v* loaded after juju-[12]*)
    • rename all functions with _juju_2_0_* prefix
  • cache TTL=2min (from 10min, see below)
  • background completion cache refresh triggered on TTL/2:
    • completion only "blocks" (waiting for actual content from eg juju status)
      if cache file is stale (>TTL)
    • join on background ready implemented via reading from coprocd fd instead of
      wait for bg process to finish, which blocks bash hard (neither responsive to Ctrl-C)
  • s/service/application/ (beta9)
@bz2
Contributor

bz2 commented Jun 22, 2016

Thanks for addressing my comments, I poked the juju team as well to get a review.
etc/bash_completion.d/juju-2.0
+${_juju_cmd_PYTHON?} -c '
+import json, sys
+sys.stderr.close()
+j=json.load(sys.stdin)
@mjs

mjs Jun 28, 2016 •
Edited 1 time

Contributor

spaces around "=" please

etc/bash_completion.d/juju-2.0
+ local cache_fname=$(_juju_2_0_juju_status_cache_fname)
+ [ -n "${cache_fname}" ] || return 0
+${_juju_cmd_PYTHON?} -c '
+trail="'${2}'"
@mjs

mjs Jun 28, 2016

Contributor

and again, and throughout

etc/bash_completion.d/juju-2.0
+j=json.load(sys.stdin)
+all_units=[]
+for k,v in j.get("applications", {}).items():
+ if v.get("units"):
@mjs

mjs Jun 28, 2016

Contributor

this called also be: all_units.extend(v.get("units", {}).keys())

@jjo

jjo Jun 28, 2016

Contributor

Thanks, changed.

etc/bash_completion.d/juju-2.0
+ if [ -z "${model}" ];then
+ model=${JUJU_MODEL:-$(${_juju_cmd_JUJU_2_0?} switch)}
+ fi
+ # echo "** comp=$(set|egrep ^COMP) ** model=${model} **">&2
@mjs

mjs Jun 28, 2016 •
Edited 1 time

Contributor

Should this commented out line still be here?

@jjo

jjo Jun 28, 2016

Contributor

Removed the commented out debugging line.

@mjs
Contributor

mjs commented Jun 28, 2016

Thanks! This looks OK to me.

For a future iteration of this, I wonder if it would be worth leaning on Python more to avoid the ugliness of doing fairly complex work in bash. I guess there's perhaps a speed trade off.
@jjo
Contributor

jjo commented Jun 28, 2016

Addressed above comments, PTAL.
etc/bash_completion.d/juju-2.0
+# juju list-models --controller <TAB>
+# juju switch <TAB>
+# juju status --model <TAB>
+# juju ssh --model <TAB> [... will complete with proper model's units/etc ...]
@mjs

mjs Jun 28, 2016

Contributor

Did you want to describe other supported completions here as well? (e.g. storage)

@mjs
Contributor

mjs commented Jun 28, 2016

LGTM
@cherylj

cherylj commented Jun 29, 2016

$$JFDI$$
@jujubot
Contributor

jujubot commented Jun 29, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 41eb737 into juju:master Jun 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment