cli: snap command migrated to docopts #160

Merged
merged 1 commit into from Dec 9, 2015

Conversation

Projects
None yet
3 participants
Collaborator

sergiusens commented Dec 7, 2015

Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

snapcraft/commands/snap.py
+ if args['DIRECTORY']:
+ # TODO: needs design
+ # TODO: overly simplistic, no meta/snap.yaml check
+ snap_dir = os.path.abspath(args['DIRECTORY'])
@elopio

elopio Dec 7, 2015

Member

This needs an integration test.

@sergiusens

sergiusens Dec 8, 2015

Collaborator

@elopio The integration tests will come in the next PR where I will enable them all again. That TODO is about snap.yaml is outdated though.

snapcraft/commands/snap.py
+ snap = lifecycle.execute('strip')
+
+ snap['arch'] = snap['arch'][0] if len(snap['arch']) == 1 else 'multi'
+ snap_name = '{name}_{version}_{arch}.snap'.format(**snap)
@elopio

elopio Dec 7, 2015

Member

I would wrap these two statements in an internal function.

@sergiusens

sergiusens Dec 8, 2015

Collaborator

got it.

snapcraft/commands/snap.py
+
+ logger.info('Snapping {}'.format(snap_name))
+ subprocess.check_call(
+ ['mksquashfs', snap_dir, snap_name, '-noappend', '-comp', 'xz'])
@elopio

elopio Dec 7, 2015

Member

a new deb dependency?

@sergiusens

sergiusens Dec 8, 2015

Collaborator

yes, and effectively moving away from the requirement to have ubuntu-snappy-cli 😄

+ return {'name': config.data['name'],
+ 'version': config.data['version'],
+ 'arch': config.data['architectures']}
+
@elopio

elopio Dec 7, 2015

Member

gentle reminders about the docstring ;)

@sergiusens

sergiusens Dec 8, 2015

Collaborator

Ah, this is the right time to do it!

Member

elopio commented Dec 7, 2015

lgtm. Minor comments, and I would like to see an integration test for snapping a dir.

Collaborator

sergiusens commented Dec 8, 2015

@elopio great, I'll write the TODO to get an integration test, but won't do it in this PR as integration tests as a whole aren't working here yet. It is just after this PR that I will overcome that issue

snapcraft/lifecycle.py
+ and after is not in this set, an exception will be raised.
+
+ :param str step: A valid step in the lifecycle: pull, build, strip or snap.
+ :raises RuntimeError: If an prerequesite of the part needs to be staged
@elopio

elopio Dec 8, 2015

Member

typo: an -> a

Member

elopio commented Dec 8, 2015

Thanks. 👍

cli: snap command migrated to docopts
Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
+
+ logger.info('Snapping {}'.format(snap_name))
+ subprocess.check_call(
+ ['mksquashfs', snap_dir, snap_name, '-noappend', '-comp', 'xz'])
@kyrofa

kyrofa Dec 8, 2015

Member

If mksquashfs exits with non-zero, this throws exceptions, right? Would it be more user-friendly to catch those here and fail gracefully, or are you intending to provide a stack trace?

@sergiusens

sergiusens Dec 9, 2015

Collaborator

@kyrofa nothing returns a stack trace by default. I learned my lesson and there is a catch all in our main execution entry point to take care of this; no matter what (I intend to add a --debug mode so such stack trace can be seen).

@sergiusens

sergiusens Dec 9, 2015

Collaborator

We can eventually catch here and throw something nice; I also don't like exposing squashfs to the build console.

sergiusens added a commit that referenced this pull request Dec 9, 2015

Merge pull request #160 from sergiusens/14-docopts-snap
cli: snap command migrated to docopts

@sergiusens sergiusens merged commit af0f89a into snapcore:new-cli Dec 9, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 83.016%
Details

@sergiusens sergiusens deleted the sergiusens:14-docopts-snap branch Dec 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment