Snap yaml #215

Merged
merged 1 commit into from Jan 11, 2016

Conversation

Projects
None yet
3 participants
Collaborator

sergiusens commented Jan 8, 2016

The documentation will be in a separate PR/commit

The bug this fixes is LP: #1532842

snapcraft/meta.py
- # TODO talk to original author if the exception to be captured here is
- # FileNotFoundError, the original code was a general catch all
- try:
+ with contextlib.suppress(FileNotFoundError):
@kyrofa

kyrofa Jan 11, 2016

Member

Would it be more clear if you simply checked for the presence of the file before removing it?

@sergiusens

sergiusens Jan 11, 2016

Collaborator

yeah, I'll change that

Member

kyrofa commented Jan 11, 2016

This looks good to me.

snapcraft/main.py
@@ -89,6 +89,7 @@ def main():
try:
commands.load(args['COMMAND']).main(argv=args['ARGS'])
except Exception as e:
+ raise e
@elopio

elopio Jan 11, 2016

Member

huh, this is weird. With this the sys exit will never happen.

@sergiusens

sergiusens Jan 11, 2016

Collaborator

yeah, escaped me.

@@ -1,6 +1,6 @@
# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
#
-# Copyright (C) 2015-2016 Canonical Ltd
+# Copyright (C) 2016 Canonical Ltd
@elopio

elopio Jan 11, 2016

Member

Why did you remove the 2015? It should have all the years that were touched.

@sergiusens

sergiusens Jan 11, 2016

Collaborator

because, not sure how it worked, but I moved meta.py to meta_legacy.py and created a new meta.py which only has 2016 things in it.

snapcraft/meta.py
-
- Returns meta_dir.
- '''
+ """Creates snap.yaml and necessary package hooks.
@elopio

elopio Jan 11, 2016

Member

s/Creates/Create

snapcraft/meta.py
+def _copy(meta_dir, relpath, new_relpath=None):
+ new_base = new_relpath or os.path.basename(relpath)
+ new_relpath = os.path.join(meta_dir, new_base)
@elopio

elopio Jan 11, 2016

Member

nit, do not overwrite the argument, use a different name for the var.

@sergiusens

sergiusens Jan 11, 2016

Collaborator

yeah, this is ... ... ... :-)

+ for app in apps:
+ for k in [k for k in ('command', 'stop-command') if k in apps[app]]:
+ execparts = shlex.split(apps[app][k])
+ apps[app][k] = _wrap_exe(execparts[0], args=execparts[1:])
@elopio

elopio Jan 11, 2016

Member

nice refactor!

@sergiusens

sergiusens Jan 11, 2016

Collaborator

thanks :-)
Simplified duplication

snapcraft/tests/test_meta.py
@@ -27,331 +26,159 @@
)
-class ComposeTestCase(tests.TestCase):
+class Create(tests.TestCase):
@elopio

elopio Jan 11, 2016

Member

CreateTestCase

- call().__enter__(),
- call().__enter__().write('my summary\nmy description\n'),
- call().__exit__(None, None, None),
- ]
@elopio

elopio Jan 11, 2016

Member

Yes, I like the removal of this :)

@sergiusens

sergiusens Jan 11, 2016

Collaborator

I thought you would like the new testing mechanism 😉

Member

elopio commented Jan 11, 2016

We are missing examples of forking daemons. I'll report a bug for this.

Collaborator

sergiusens commented Jan 11, 2016

I've updated the review taking into account the comments from @kyrofa and @elopio
I've also added the bug marker to the the commit message

Member

elopio commented Jan 11, 2016

Looks good to me too.

Member

elopio commented Jan 11, 2016

And bug reported about the forking: https://bugs.launchpad.net/snapcraft/+bug/1532853

sergiusens added a commit that referenced this pull request Jan 11, 2016

@sergiusens sergiusens merged commit 89904e3 into snapcore:master Jan 11, 2016

2 checks passed

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

@sergiusens sergiusens deleted the sergiusens:snap_yaml branch Jan 11, 2016

smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016

fix for #127. try to use posix.fallocate and fallocate before falling…
… back to dd; adds unit tests for basic functionality and shellutil.quote (#215)

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

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