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

Avoid changing default endpoint fixes #346 #347

Merged

Conversation

jvrsantacruz
Copy link
Contributor

When an alternative lxd server is installed via snap, pylxd will change the default to it, effectively
switching from the current server to a different one without the user knowing.

This was making the default value of the Client unreliable as it might change anytime without warning.

Applications relying on the default value of the endpoint for the Client had to be forced now to set LXD_DIR to avoid sudden changes when users get a side installation to play with.

@codecov-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #347 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   96.83%   96.83%   +<.01%     
==========================================
  Files          12       12              
  Lines         980      981       +1     
  Branches      110      110              
==========================================
+ Hits          949      950       +1     
  Misses         12       12              
  Partials       19       19
Impacted Files Coverage Δ
pylxd/client.py 98.7% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad07da9...97c99ca. Read the comment docs.

pylxd/client.py Outdated
else:
path = '/var/lib/lxd/unix.socket'
path = os.path.join(
os.getenv('LXD_DIR', '/var/lib/lxd'), 'unix.socket')
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, but the choose the snap version needs to stay. There is merit in a conversation about whether to check for the /var/lib/lxd/unix.socket first, and defaulting to that, rather than the snap, but the pylxd library will fail to work in a snap only installed lxd environment which is the default in 18.10 and above (on Ubuntu).

Signed-off-by: Javier Santacruz <javier.santacruz@avature.net>
@jvrsantacruz
Copy link
Contributor Author

Hi,

Thanks for reviewing this. I wasn't aware that the snap installation was about to be the single one.
As to this date we used it to install alternative or modern versions and test.

I rewrote the previous commit to look for /var/lib/lxd/unix.socket before going for /var/snap/lxd/common/lxd/unix.socket. If that's more in line with what's expected of the library.

Thank you!

@ajkavanagh
Copy link
Contributor

Thanks for the update; I'm reviewing and testing this. I think it's probably okay to include this functionality, but I need to check that it works after an lxd.migrate.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

This looks good, and my testing against pkg and snap versions is fine. Thanks for the changes.

@ajkavanagh ajkavanagh merged commit 9373004 into canonical:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants