/snapcraft

Integrate with new remote parts #590

Merged
merged 13 commits into from Jun 24, 2016
+183 −214

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@sergiusens @elopio
@sergiusens
Collaborator

sergiusens commented Jun 22, 2016

No description provided.

snapcraft/internal/parts.py
+ :param str part_name: The name of the part to query from the wiki
+ :param dict properties: The current set of properties
+ :return: Part properties from the wiki composed with the properties
+ passed as a parameter. If there is no wiki part named name,
@elopio

elopio Jun 23, 2016

Member

What about:
If there is no wiki part with the name passed as parameter, just the properties parameter will be returned.

@sergiusens

sergiusens Jun 23, 2016

Collaborator

I actually need to remove this as we a return a KeyError instead ;-)

snapcraft/internal/parts.py
+ passed as a parameter. If there is no wiki part named name,
+ properties will be returned.
+ :rtype: dict
+ :raises KeyError: if the part named name is not found in the wiki.
@elopio

elopio Jun 23, 2016

Member

I would just write: if the part is not found in the wiki.

@sergiusens

sergiusens Jun 23, 2016

Collaborator

done

snapcraft/internal/yaml.py
@@ -144,14 +144,12 @@ def _process_parts(self):
plugin_name = properties.pop('plugin', None)
if not plugin_name:
logger.info(
- 'Searching the wiki to compose part "{}"'.format(
- part_name))
+ 'Searching in the remotes parts cache for part '
@elopio

elopio Jun 23, 2016

Member

s/remotes/remote/

@elopio
Member

elopio commented Jun 23, 2016

I think the mosquitto example is failing here because it is not defined in the new wiki.
Also there is this integration test error: ======================================================================
ERROR: test_pull_wiki_part (test_wiki.WikiTestCase)

test_wiki.WikiTestCase.test_pull_wiki_part

testtools.testresult.real._StringException: output: {{{
Searching in the remotes parts cache for part 'curl'
Issues while validating snapcraft.yaml: the "plugin" keyword is missing for the "curl" part.
}}}

Seems legit failure.
snapcraft/internal/yaml.py
+ 'in the defined remote parts, {!r} is missing the '
+ '`plugin` entry and is not defined in the current '
+ 'remote parts cache, try to run `snapcraft update` '
+ 'to refresh'.format(part_name))
@elopio

elopio Jun 24, 2016

Member

This is hard to read, three commas in a sentence. I would remove the first sentence and just say: {!r} is not defined in the current remote parts cache, try to run snapcraft update to refresh. And maybe add a help entry were we explain more? So we can finish the message with: Type snapcraft help remote-parts for more information.
Just my opinion, I'm not sure my proposal is better.

snapcraft/internal/yaml.py
- # The wiki still supports using 'type' for snapcraft 1.x
- if 'type' in properties:
- del properties['type']
+ except KeyError:
@elopio

elopio Jun 24, 2016

Member

There is no test for this exception.

snapcraft/internal/yaml.py
+ 'in the defined remote parts, {!r} is missing the '
+ '`plugin` entry and is not defined in the current '
+ 'remote parts cache, try to run `snapcraft update` '
+ 'to refresh'.format(part_name))
if not plugin_name:
raise PluginNotDefinedError(part_name)
@elopio

elopio Jun 24, 2016

Member

this is also missing coverage. Seems closely related to your branch, but feel free to report it as an error to be fixed later.

snapcraft/internal/yaml.py
self._part_names.append(dep)
+ else:
+ logger.info('Maybe {!r} is defined as a remote part, '
+ 'run `snapcraft update` to refresh')
@elopio

elopio Jun 24, 2016

Member

also missing a test. And should this be an error, shouldn't it?
This logic is so weird. I think this else should raise the exception.
And I don't have it clear if in that case, the following statement is needed.
Splitting smaller functions with nice names out of these loops would be nice, but again, maybe for a future refactor.

@sergiusens

sergiusens Jun 24, 2016

Collaborator

El 24/06/16 a las 03:04, Leo Arias escribió:

In snapcraft/internal/yaml.py
ubuntu-core#590 (comment):

                     self._part_names.append(dep)

  •                else:

  •                    logger.info('Maybe {!r} is defined as a remote part, '

  •                                'run `snapcraft update` to refresh')

also missing a test. And should this be an error, shouldn't it?
This logic is so weird. I think this else should raise the exception.
And I don't have it clear if in that case, the following statement is
needed.
Splitting smaller functions with nice names out of these loops would be
nice, but again, maybe for a future refactor.

I couldn't agree more, tis is all horrible legacy code that needs some love (all of snapcraft.internal.yaml)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ubuntu-core/snapcraft/pull/590/files/a60b0459d698406fc3c67017a4c1009550f38566#r68357000,
or mute the thread
https://github.com/notifications/unsubscribe/ABF_5ZWIuEVRP5YigYeqE-UeqU8CXCZVks5qO3N7gaJpZM4I7W40.

@sergiusens

sergiusens Jun 24, 2016

Collaborator

https://bugs.launchpad.net/snapcraft/+bug/1596044

sergiusens added some commits Jun 22, 2016

@sergiusens
Integrate with the new remote parts 
These replace the wiki.

LP: #1594976

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
6107ded
@sergiusens
Update the downloader wiki test
2ee799e
@sergiusens
Update docstrings
35b0a55
@sergiusens
Add a nicer error message for missing remote parts 
Also fix the integration test
f9beb7b
@sergiusens
Bringing back the old wiki to my own detriment
63edf35
@sergiusens
meh
dc44746
@sergiusens
Revert "meh" 
This reverts commit c023734.
07d8726
@sergiusens
Revert "Bringing back the old wiki to my own detriment" 
This reverts commit ff629c0.
163e1f1
@sergiusens
Use new wiki entry
2a32f39
@sergiusens
oh meld..
7f08e83
@sergiusens
Update tests and error logic

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 96.13%
Details
633852a
@elopio
Member

elopio commented Jun 24, 2016

👍

sergiusens added some commits Jun 24, 2016

@sergiusens
grammar

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 96.13%
Details
14e1cec
@sergiusens
Merge branch 'master' into feature/1594976/wiki-replacement

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 96.192%
Details
490ac01

@sergiusens sergiusens merged commit 9c9fb69 into snapcore:master Jun 24, 2016
2 checks passed

2 checks passed

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

@sergiusens sergiusens deleted the sergiusens:feature/1594976/wiki-replacement branch Jun 24, 2016

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

@sergiusens
Integrate with new remote parts (#590) 
These replace the wiki.

LP: #1594976

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
0f3e5e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment