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: snapcraft refresh in containers #1412
Conversation
kalikiana
referenced this pull request
Jul 17, 2017
Merged
lxd: Inject snapcraft and core snaps into the container #1364
kalikiana
self-assigned this
Jul 19, 2017
kalikiana
requested a review
from
sergiusens
Jul 19, 2017
kalikiana
added some commits
Jul 27, 2017
sergiusens
added this to the 2.34 milestone
Aug 3, 2017
kalikiana
changed the title from
lxd: Snapcraft update in containers
to
lxd: snapcraft update in containers
Aug 8, 2017
kalikiana
added some commits
Aug 8, 2017
sergiusens
changed the title from
lxd: snapcraft update in containers
to
lxd: snapcraft refresh in containers
Aug 30, 2017
sergiusens
modified the milestones:
2.35,
2.34
Sep 6, 2017
kalikiana
added some commits
Sep 7, 2017
kyrofa
reviewed
Sep 8, 2017
Looking good, few suggestions though, particularly regarding terminology and where the refresh feature landed.
| +@lifecyclecli.command() | ||
| +@click.option('--debug', is_flag=True, | ||
| + help='Shells into the environment if the build fails.') | ||
| +def refresh(debug, **kwargs): |
kyrofa
Sep 8, 2017
Member
This isn't part of the lifecycle. In order to prevent this module from becoming a catchall, I suggest you create a new module.
| + "SNAPCRAFT_CONTAINER_BUILDS is not set or 0.\n" | ||
| + "Maybe you meant to update the parts cache instead? " | ||
| + "You can do that with the following command:\n\n" | ||
| + "snapcraft update") |
| + project_options = get_project_options(**kwargs) | ||
| + lifecycle.containerbuild('update', project_options) | ||
| + else: | ||
| + remote_parts.update() |
kalikiana
Sep 12, 2017
Collaborator
Same forum topic, arguably separate fix. I dropped it from this one and proposed #1546.
| try: | ||
| - self._container_run(command, cwd=self._project_folder) | ||
| + if step == 'refresh': |
kyrofa
Sep 8, 2017
Member
Again, not a lifecycle step. Please change terminology or refactor this function a bit.
kalikiana
added some commits
Sep 12, 2017
|
Whoa, something weird is happening here, any ideas? I didn't even get far enough to test this. Logged LP: #1716738.
|
kyrofa
dismissed
their
stale review
Sep 12, 2017
Changes look good, needs testing now.
|
Note that I get the same issues just running |
kalikiana
added some commits
Sep 13, 2017
kalikiana
dismissed
sergiusens’s
stale review
Oct 4, 2017
New logic has been split into a refresh command as discussed
kalikiana
referenced this pull request
Oct 12, 2017
Merged
cli: update parts cache in the container #1546
|
|
|
Closing for now - will update and re-open once #1627 is in. |
kalikiana
closed this
Oct 23, 2017
|
#1627 is in, now. |
kalikiana
reopened this
Nov 3, 2017
|
@kalikiana did this have thorough testing on your side? |
|
I did some random testing of this. For more reliable results a log of specific steps tested just now on
|
| + | ||
| + \b | ||
| + Examples: | ||
| + SNAPCRAFT_CONTAINER_BUILDS=1 snapcraft refresh |
sergiusens
Nov 8, 2017
Collaborator
Can you omit SNAPCRAFT_CONTAINER_BUILDS and instead make this command optionally show up by means of doing
if not os.environ.get('SNAPCRAFT_CONTAINER_BUILDS'):
command.pop(commands.index('refresh'))Right after https://github.com/snapcore/snapcraft/blob/master/snapcraft/cli/__init__.py#L92
kalikiana
Nov 8, 2017
Collaborator
Done. I used use_container here since eventually it won't be just the environment variable.
kalikiana commentedJul 17, 2017
•
Edited 1 time
-
kalikiana
Aug 25, 2017
See the forum proposal.
Note: The other part is implemented in #1546.