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

add SNAP bin path to bash completion #6927

Merged
merged 2 commits into from Feb 7, 2017

Conversation

adam-stokes
Copy link

For snapped versions of Juju which reside in /snap/bin/juju we need to make bash
completion aware of this additional binary path.

Signed-off-by: Adam Stokes battlemidget@users.noreply.github.com

Please provide the following details to expedite Pull Request review:


Description of change

This change is needed to support an additional $PATH for snapped version of Juju.

QA steps

A new snap version of Juju or conjure-up needs to be built and then you can juju to verify completion still works.

Documentation changes

This does not require additional documentation.

Bug reference

No bug exists for this addition.

For snapped versions of Juju which reside in /snap/bin/juju we need to make bash
completion aware of this additional binary path.

Signed-off-by: Adam Stokes <battlemidget@users.noreply.github.com>
Copy link
Contributor

@anastasiamac anastasiamac left a comment

Choose a reason for hiding this comment

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

With these changes, snap will take priority over installed Juju version. Just to be clear :)

If this is the intent, we should flag this with @juju/docs and release team @nskaggs

@nskaggs
Copy link
Contributor

nskaggs commented Feb 7, 2017

/usr/bin should probably come first. I wonder if we can just say which juju however?

@adam-stokes
Copy link
Author

adam-stokes commented Feb 7, 2017

I'm not sure which juju I would think that would be similar to doing /usr/bin/env. What's the overall consensus on using that version the absolute path?

@nskaggs updated based on your suggestion, I'm still not sure about using which juju so I stuck with absolute paths.

Make sure /usr/bin/juju* has the priority over a snapped version.

Signed-off-by: Adam Stokes <battlemidget@users.noreply.github.com>
@jameinel
Copy link
Member

jameinel commented Feb 7, 2017

Wouldn't it depend on what your $PATH is set to? (is it /usr/bin;/snap/bin; or is it /snap/bin;/usr/bin ?)
On my system without personal tweaking (AFAIK) it is:
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin

So IMO, we should definitely prefer /usr/bin over /snap/bin. Because otherwise, if you have both installed you'll do "juju " and it will actually be /usr/bin/juju but you'll be doing tab completion on /snap/bin/juju, right?

@adam-stokes
Copy link
Author

@jameinel Aside from the fallback to /usr/bin/juju the order should be correct now. I don't think we should have the catch all be /snap/bin/juju though. I also took at look at $PATH on several vanilla installs and /snap/bin is the last item in $PATH.

@nskaggs
Copy link
Contributor

nskaggs commented Feb 7, 2017

@jameinel right. Ubuntu sets path priority to debs over snaps. We should mirror the same here. Thanks.

@nskaggs
Copy link
Contributor

nskaggs commented Feb 7, 2017

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Feb 7, 2017

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

@jujubot jujubot merged commit 71ece57 into juju:2.1 Feb 7, 2017
@adam-stokes adam-stokes deleted the 2.1-update-bash-comp branch February 7, 2017 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants