Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bash completions #6499

Merged
merged 2 commits into from Oct 26, 2016

Conversation

dimitern
Copy link

@dimitern dimitern commented Oct 25, 2016

Makefile's install-etc was not installing juju-version
Changed the completions to work for a juju binary, built
from source, as well as the one from the juju-2.0 package.

See bug http://pad.lv/1622775 for more details
(PR was intended to fix it, but simply improves CLI UX)

QA steps:

  1. make install-etc && source /etc/bash_completion
  2. juju boo[tab]tstrap
  3. juju de[tab]p[tab]loy ubuntu
  4. juju status [tab]u[tab]buntu/0

Copy link
Contributor

@bz2 bz2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! See comments for a couple of suggested changes.

for k, v in j.get("applications", {}).items():
all_units.extend(v.get("units", {}).keys())
print ("\n".join([unit + trail for unit in all_units]))
except: pass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want try:/except: pass - yes the kipple when things break is bad, but things being broken with no feedback as to why is worse I feel.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Dropped (all places).


# Detect juju built from source (highest priority)
if [ -x "$GOPATH/bin/juju" ]; then
export _juju_cmd_JUJU_2_0="$GOPATH/bin/juju"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good. Might mean developers see packaging issues less, but in exchange will be catching their own problems instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, and makes completions work for devs as well as other users :)

@@ -99,7 +99,8 @@ endif
# Install bash_completion
install-etc:
@echo Installing bash completion
@sudo install -o root -g root -m 644 etc/bash_completion.d/juju2 /etc/bash_completion.d
@sudo install -o root -g root -m 644 etc/bash_completion.d/juju-2.0 /etc/bash_completion.d
@sudo install -o root -g root -m 644 etc/bash_completion.d/juju-version /etc/bash_completion.d
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These locations should probably be updated to the new /usr/share/bash-completion/bash_completion location. The split between /etc and there is also the cause of much confusion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Done.

@dimitern
Copy link
Author

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Oct 26, 2016

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

@jujubot jujubot merged commit c51929f into juju:develop Oct 26, 2016
@dimitern dimitern deleted the lp-1622775-bash-completion-traceback branch October 26, 2016 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants