Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[modified] Added an attribute for defining a destination directory. #210
Conversation
|
I'm having trouble understanding why this change is necessary given we have all the organization keywords. Can you please explain? |
|
What do you mean with "organization keywords"? It has to do with this On 8 January 2016 at 14:38, Kyle Fazzari notifications@github.com wrote:
|
|
@rbreitenmoser please read our contributing guidelines. You're missing a few steps here. |
|
@kyrofa I saw, that I didn't made a feature branche on my fork. Should I make a new pull request and then you can delete this one? The Canonical Contributor Agreement should now be ok. Some tests are missing (coverage decreased) |
rbreitenmoser
closed this
Jan 9, 2016
rbreitenmoser
reopened this
Jan 9, 2016
|
@rbreitenmoser you need to make sure your launchpad account is associated with the email address you used to commit, or we can't verify that you signed the CLA. Regarding the feature branch, no big deal. Just keep it in mind for the future. |
|
@kyrofa I added the email address, which I used to commit, also to my launchpad account. I hope it works now. |
sergiusens
reviewed
Jan 12, 2016
| @@ -0,0 +1,84 @@ | ||
| +# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| +# | ||
| +# Copyright (C) 2015 Canonical Ltd |
|
Verified CLA. |
|
@rbreitenmoser please update the date as @sergiusens mentioned, and squash. Make sure the LP bug reference is on a line by itself in the commit message, after the title. |
|
Something went wrong with the squash. I will try to fix it tomorrow |
|
@kyrofa finally I got it :-) Everything squashed to one commit |
|
@rbreitenmoser heh, you're so close! Your commit actually needs a meaningful title though-- swap the two lines, putting the LP bug reference on the second line. Instead of saying what needs to happen, say what the commit does, e.g. "tar-content plugin: Add ability to unpack into a specific destination directory" or something. |
kyrofa
reviewed
Jan 25, 2016
| + raise ValueError('path "{}" must be relative' | ||
| + .format(self.options.destination)) | ||
| + | ||
| + builddir = self.builddir + '/' + self.options.destination |
kyrofa
reviewed
Jan 25, 2016
| + if not self.options.destination: | ||
| + installdir = self.installdir | ||
| + else: | ||
| + installdir = self.installdir + '/' + self.options.destination |
kyrofa
reviewed
Jan 25, 2016
| @@ -1,6 +1,6 @@ | ||
| # -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*- | ||
| # | ||
| -# Copyright (C) 2015 Canonical Ltd | ||
| +# Copyright (C) 2016 Canonical Ltd |
|
@kyrofa I updated everything based on your feedback. The travis build failed now, but I think id's not because of me. travis --> W: Failed to fetch https://apt.dockerproject.org/repo/dists/ubuntu-trusty/main/binary-amd64/Packages gnutls_handshake() failed: Handshake failed How can I restart the build without committing? |
|
I restarted it for you. |
|
@kyrofa still no luck.... |
|
|
elopio
reviewed
Jan 26, 2016
| @@ -49,6 +50,8 @@ def test_stage_nil_plugin(self): | ||
| expected_dirs = [ | ||
| 'dir-simple', | ||
| 'notopdir', | ||
| + 'destdir1', | ||
| + os.path.join('destdir1','destdir2') | ||
| ] | ||
| for expected_dir in expected_dirs: | ||
| self.assertThat( |
elopio
reviewed
Jan 26, 2016
| + if not os.path.exists(installdir): | ||
| + os.makedirs(installdir) | ||
| + | ||
| + self.tar.provision(installdir) |
elopio
reviewed
Jan 26, 2016
| @@ -40,7 +40,8 @@ def test_stage_nil_plugin(self): | ||
| 'notop', | ||
| 'parent', | ||
| 'slash', | ||
| - 'readonly_file' | ||
| + 'readonly_file', | ||
| + os.path.join('destdir1','destdir2','top-simple') |
elopio
Jan 26, 2016
Member
You are missing spaces between the commas here. I wonder why the static tests are not catching this pep8 error.
os.path.join('destdir1', 'destdir2', 'top-simple')
elopio
reviewed
Jan 26, 2016
| @@ -49,6 +50,8 @@ def test_stage_nil_plugin(self): | ||
| expected_dirs = [ | ||
| 'dir-simple', | ||
| 'notopdir', | ||
| + 'destdir1', | ||
| + os.path.join('destdir1','destdir2') |
elopio
reviewed
Jan 26, 2016
| + def setUp(self): | ||
| + super().setUp() | ||
| + # setup the expected target dir in our tempdir | ||
| + self.build_prefix = 'parts/tarContent/build/' |
elopio
Jan 26, 2016
Member
just a nit, because it's not common to find camel case in python code, I would write this as tar_content.
elopio
reviewed
Jan 26, 2016
| + # setup the expected target dir in our tempdir | ||
| + self.build_prefix = 'parts/tarContent/build/' | ||
| + os.makedirs(self.build_prefix) | ||
| + self.install_prefix = 'parts/tarContent/install/' |
elopio
reviewed
Jan 26, 2016
| + TarContentPlugin('tarContent', Options()) | ||
| + | ||
| + self.assertEqual(raised.exception.__str__(), | ||
| + "path \"/destdir1\" must be relative") |
elopio
Jan 26, 2016
Member
Using single quotes here is a little more readable.
'path "/destdir1" must be relative'
elopio
reviewed
Jan 26, 2016
| + | ||
| + # create tar file for testing | ||
| + os.mkdir('src') | ||
| + fileToTar = os.path.join('src', 'test.txt') |
elopio
reviewed
Jan 26, 2016
| + tar.close() | ||
| + | ||
| + t = TarContentPlugin('tarContent', Options()) | ||
| + t.build() |
elopio
Jan 26, 2016
Member
one letter variables are not common in practice. I would call this plugin instead of t, or chain the build call, so you don't have to assign it to anything:
TarContentPlugin('tarContent', Options()).build()
|
lgtm. I just left some pain-in-the-ass style comments. |
|
@elopio no problem. I updated everything based on your feedback. |
|
Excellent, thank you @rbreitenmoser! |
added a commit
that referenced
this pull request
Jan 26, 2016
kyrofa
merged commit c10327c
into
snapcore:master
Jan 26, 2016
pushed a commit
to smoser/snapcraft
that referenced
this pull request
Sep 14, 2016
pushed a commit
to kalikiana/snapcraft
that referenced
this pull request
Apr 6, 2017
rbreitenmoser commentedJan 8, 2016
The tar will be untared to the defined directory in the ../build and
../install directory from the snap. The new attribute "destination" is optional.
Extract from the spancraft.yaml
destination-dir:
plugin: tar-content
source: simple.tar.bz2
destination: destdir1/destdir2