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.

Sign up for GitHub

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

Jump to bottom

cli: Containerbuild clean #1372

Merged
merged 4 commits into from
Jul 3, 2017
Merged

cli: Containerbuild clean #1372

merged 4 commits into from
Jul 3, 2017

Conversation

kalikiana
Copy link
Contributor

Add a unit test (and fix) for SNAPCRAFT_CONTAINER_BUILDS=1 clean.

@kalikiana kalikiana requested a review from come-maiz June 21, 2017 11:59
@kalikiana kalikiana self-assigned this Jun 21, 2017
@kalikiana kalikiana added the bug Actual bad behavior that don't fall into maintenance or documentation label Jun 21, 2017
@kalikiana kalikiana requested a review from kyrofa June 22, 2017 12:26
@come-maiz
Copy link
Contributor

This looks correct to me. However, I'm having problems to test it.

I'm failing to build the snap with this error: http://paste.ubuntu.com/24930727/

Then the container is stopped. When I run clean, the first thing it tries to do is to start it, and it fails.

@kalikiana do you see something wrong in my first snapcraft call? And in the second, wouldn't it be better not even trying to start the container? just remove it?

come-maiz
come-maiz reviewed Jun 23, 2017
View reviewed changes
snapcraft/internal/lxd.py Outdated
@@ -219,7 +219,7 @@ def _finish(self):

def execute(self, step='snap', args=None):
super().execute(step, args)
if step == 'clean' and not args:
if step == 'clean' and 'pull' in args:
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we have to check here if there are no parts in args? Maybe, this clean command is to clean only one part, and leave the rest.

@kalikiana
Copy link
Contributor Author

@ElOpio I have no idea what your error is about. I suggest you open a forum topic for investigation. This branch isn't changing the implementation, only the passing of arguments.
I don't think we can "just" remove the container because then we wouldn't be cleaning anything in the project folder. But, this would also be a nice forum topic. We can maybe find ways to optimize by cleaning outside of the container (and investigate potential problems with that).

@come-maiz
Copy link
Contributor

@kalikiana there's clearly a bug, not introduced by this branch but it prevents me from testing this branch. I reported it here, with simple steps to reproduce: https://bugs.launchpad.net/snapcraft/+bug/1700703

It happens on all my machines, but I made sure to reproduce it in a clean environment. I assigned it to you. Please take a look.

@kalikiana kalikiana requested a review from sergiusens July 3, 2017 11:33
sergiusens
sergiusens approved these changes Jul 3, 2017
View reviewed changes
Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

works ok on my local system, the lxd weirdness I saw elsewhere remains to be seen.

@sergiusens sergiusens added this to the 2.33 milestone Jul 3, 2017
@sergiusens sergiusens merged commit 9536f78 into canonical:master Jul 3, 2017
kalikiana added a commit to kalikiana/snapcraft that referenced this pull request Aug 3, 2017
@kalikiana
core: clean the container on a full clean (canonical#1372)
975f4d2 
Add a unit test (and fix) for `SNAPCRAFT_CONTAINER_BUILDS=1 clean`
kalikiana added a commit to kalikiana/snapcraft that referenced this pull request Sep 21, 2017
@kalikiana
core: clean the container on a full clean (canonical#1372)
3f15c13 
Add a unit test (and fix) for `SNAPCRAFT_CONTAINER_BUILDS=1 clean`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-maiz come-maiz come-maiz left review comments

@sergiusens sergiusens sergiusens approved these changes

@kyrofa kyrofa Awaiting requested review from kyrofa

Assignees

@kalikiana kalikiana

Labels
bug Actual bad behavior that don't fall into maintenance or documentation
Projects
None yet
Milestone
2.33
Development

Successfully merging this pull request may close these issues.
3 participants
@kalikiana @come-maiz @sergiusens