/snapcraft

Fixed assets go into a setup dir #399

Merged
merged 1 commit into from Mar 23, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@sergiusens @coveralls @elopio @kyrofa @zyga
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

Instead of declaring fixed assets we put them into a setup dir.
Implementation for gui (including icon) and license provided.

LP: 1560969

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
@sergiusens
Fixed assets go into a setup dir 
Instead of declaring fixed assets we put them into a `setup` dir.
Implementation for gui (including icon) and license provided.

LP: 1560969

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

All checks have passed

4 successful checks
@snappy-m-o
Examples tests — Success 14 tests run, 0 skipped, 0 failed.
Details
@snappy-m-o
autopkgtest — Success No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 95.518%
Details
b6ad3ad
@coveralls

coveralls commented Mar 23, 2016

Coverage Status

Coverage increased (+0.05%) to 95.518% when pulling b6ad3ad on sergiusens:feature/1560969/gui into 889fe87 on ubuntu-core:master.
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

retest this please
@elopio
Member

elopio commented Mar 23, 2016

retest this please
@kyrofa
Member

kyrofa commented Mar 23, 2016

👍 from me, though we may want a snappy review.
@elopio
Member

elopio commented Mar 23, 2016

the branch is ok, but it's missing some docs explaining the use of the new directory.
In the PITA side of things, I don't think "setup" is a good name, because it is not setting up anything. Something like "assets" would make more sense in my opinion. Of course, I don't want to start a new discussion about terminology if this term was already discussed and comes from one of the specifications.
@kyrofa
Member

kyrofa commented Mar 23, 2016

Good point @elopio, "assets" does seem more appropriate.
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

@elopio @kyrofa this term comes from specifications. There's also an email thread where this was discussed.
@kyrofa
Member

kyrofa commented Mar 23, 2016

@sergiusens alright, that works.
snapcraft/meta.py
+
+ license_src = os.path.join(setup_dir, 'license.txt')
+ license_dst = os.path.join(meta_dir, 'license.txt')
+ if os.path.exists(license_dst) and os.path.exists(license_src):
@zyga

zyga Mar 23, 2016

Contributor

This could be moved under the subsequent test if os.path.exists(license_src) and simplified to just if os.path.exists(license_dst), that is easier to read for me.

@sergiusens

sergiusens Mar 23, 2016

Collaborator

right, thanks for the suggestion, it feels like personal taste; I don't like nesting ifs; but your solution would be more performant.

@zyga

zyga Mar 23, 2016

Contributor

Don't feel blocked on this, I just found it easier to read this way.

snapcraft/meta.py
+
+ gui_src = os.path.join(setup_dir, 'gui')
+ gui_dst = os.path.join(meta_dir, 'gui')
+ if os.path.exists(gui_dst) and os.path.exists(gui_src):
@zyga

zyga Mar 23, 2016

Contributor

Same as below for license

snapcraft/meta.py
+ _setup_from_setup(meta_dir)
+
+
+def _setup_from_setup(meta_dir):
@zyga

zyga Mar 23, 2016

Contributor

This looks okay but I'd like @mvo5 to ack it as well. I'm not 100% sure of the design of the setup directory.

@zyga
Contributor

zyga commented Mar 23, 2016

+1 but please get an ack from @mvo5 as I'm not 100% sure about setup/
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

@elopio @kyrofa @zyga https://lists.ubuntu.com/archives/snappy-devel/2016-March/001598.html

@zyga I don't think @mvo5 was part of the origin directory; I just want to make sure someone from the snappy team acks I'm putting stuff in the right target directory 😉
@sergiusens
Collaborator

sergiusens commented Mar 23, 2016

I've already spotted many places where docs are lacking or plain 15.04 incorrect; I'll work on a general branch for that.

@sergiusens sergiusens merged commit 5a099d2 into snapcore:master Mar 23, 2016
4 checks passed

4 checks passed

Examples tests Success 14 tests run, 0 skipped, 0 failed.
Details
autopkgtest Success No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 95.518%
Details

@sergiusens sergiusens deleted the sergiusens:feature/1560969/gui branch Mar 23, 2016

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

@hglkrijger
ensure all calls to call_storage_service use http proxy; pep8 fixes a… 
…nd cleanup (#399)
91d64b5

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

@sergiusens
Merge pull request #399 from sergiusens/feature/1560969/gui 
Fixed assets go into a setup dir
2333c2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment