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

Use architectures field which existed in older LXD #1305

Merged
merged 3 commits into from
May 12, 2017

Conversation

kalikiana
Copy link
Contributor

LXD recently renamed the kernelarchitecture field to kernel-architecture so snapcraft is unwittingly not backwards-compatible.

This was filed as bug 1689712.

@@ -55,7 +55,7 @@ def __init__(self, *, output, source, project_options,
_verify_remote(remote)
self._container_name = '{}:snapcraft-{}'.format(remote, container_name)
# Use the server architecture to avoid emulation overhead
kernel = self._get_remote_info()['environment']['kernel_architecture']
kernel = self._get_remote_info()['environment']['architectures'][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is riskier I think.
How do you know that the first architecture is always the kernel architecture? And I'm not sure if yaml.load guarantees the order in lists.
I was thinking of just trying with both values, like:

   try:
      kernel = self._get_remote_info()['environment']['kernel_architecture']
   except KeyError:
      kernel = self._get_remote_info()['environment']['kernelarchitecture']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know because that's what LXD does in the source code. But I have no strong opinion one way or the other, so I'm happy to go for the fallback instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are sure this is not flaky, I'm ok with the approach that you prefer.

@codecov-io
Copy link

codecov-io commented May 10, 2017

Codecov Report

Merging #1305 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1305      +/-   ##
==========================================
- Coverage   95.26%   95.25%   -0.01%     
==========================================
  Files         224      224              
  Lines       20792    20796       +4     
  Branches     1662     1662              
==========================================
+ Hits        19808    19810       +2     
- Misses        682      684       +2     
  Partials      302      302
Impacted Files Coverage Δ
snapcraft/internal/lxd.py 87.67% <60%> (-1.07%) ⬇️

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 95d737d...496c488. Read the comment docs.

@sergiusens sergiusens merged commit 8f15f69 into canonical:master May 12, 2017
@sergiusens sergiusens added this to the 2.30 milestone May 12, 2017
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.

None yet

4 participants