Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
lxd: don't inject local snaps on a different arch #1577
Conversation
kalikiana
added some commits
Sep 29, 2017
kalikiana
self-assigned this
Sep 29, 2017
kalikiana
added some commits
Oct 5, 2017
elopio
reviewed
Oct 10, 2017
-1 because of the test that returns before the assertion.
What about a manual test here?
| + # Switch channel if needed | ||
| + cmd[1] = 'refresh' | ||
| + self._container_run(cmd) | ||
| + return |
kalikiana
Oct 10, 2017
Collaborator
Good idea actually. I refactored it now, and the file-based install also uses it.
| 'revision': 'x1'}, | ||
| ] | ||
| builder = self.make_cleanbuilder() | ||
| builder.execute() | ||
| + if hasattr(self, 'cross') and self.cross: | ||
| + return |
elopio
Oct 10, 2017
Member
I don't understand this return. Should it be a skip? A test without assertion is wrong.
kalikiana
Oct 10, 2017
Collaborator
I was thinking the existing assertions didn't make sense... I added different ones now.
kalikiana
Oct 11, 2017
Collaborator
I figured it's redundant since it's the same in both cases... which in hindsight I should've said in the comment. I added it now anyway.
kalikiana
added some commits
Oct 10, 2017
| + if (self._project_options.target_arch | ||
| + and self._project_options.target_arch != self._server_arch): | ||
| + channel = json['result']['channel'] | ||
| + return self._install_snap(name, channel, is_classic=is_classic) |
sergiusens
Oct 16, 2017
Collaborator
eventually this code would need some refactoring, these return statements in the middle will bite us back later
| + self._container_run(['snap', 'install', name] + args) | ||
| + if channel: | ||
| + # Switch channel if needed | ||
| + self._container_run(['snap', 'refresh', name] + args) |
kyrofa
Oct 10, 2017
Member
I'm curious why we're doing this in a step separated from the install. I guess I have two questions:
- Why not just use
--channel=<cannel>in the install? - What do we expect to happen if we refresh to (or install with) a channel when installing with
--dangerous?
kalikiana
Oct 11, 2017
Collaborator
I updated the comments in an attempt to clarify: snap install will do nothing if the snap's already installed, even if the channel is different. So the refresh is needed to switch it - it won't do anything if the channel is the same. That's just how the command works...
There is no case where --dangerous is used with a channel - if it's a different arch and we snap install by name plus channel, there's no reasonable way to push an x-revisioned snap then, so we use the regular one. Does this make sense to you?
kyrofa
Oct 11, 2017
Member
Ah, I see-- I actually missed that you were also passing the channel to install. Carry on.
| 'revision': 'x1'}, | ||
| ] | ||
| builder = self.make_cleanbuilder() | ||
| builder.execute() | ||
| + if hasattr(self, 'cross') and self.cross: | ||
| + return |
elopio
Oct 10, 2017
Member
I don't understand this return. Should it be a skip? A test without assertion is wrong.
kalikiana
Oct 10, 2017
Collaborator
I was thinking the existing assertions didn't make sense... I added different ones now.
kalikiana
Oct 11, 2017
Collaborator
I figured it's redundant since it's the same in both cases... which in hindsight I should've said in the comment. I added it now anyway.
kalikiana
added some commits
Oct 11, 2017
|
Failing with a false negative |
kalikiana
dismissed
elopio’s
stale review
Oct 12, 2017
Tests were added
sergiusens
approved these changes
Oct 16, 2017
this lxd module seems like refactor ready to allow it to grow further, but this does solve the problem
kalikiana commentedSep 29, 2017
•
Edited 1 time
-
kalikiana
Oct 5, 2017
./runtests.sh static?./runtests.sh unit?As brought up in the forum when running the Snapcraft snap with a remote that has a different architecture it will try to push local snaps that can't be installed. For instance, local x86-64 machine, remote Raspberry Pi, which is armhf. In that case we have to install the snaps from the store instead.