Allow source-type to specify local #1352
Conversation
|
Hey, thanks for this, if no test can we at least get a reason for needing this? |
|
Sure, I put a reason on the LP bug, but will reproduce here. "The recent change to the schema that added an enumeration of valid 'source-types' means that now you cannot specify source-type to be local." "The particular use case this allows is when you have a remote part with one of the currently valid source types e.g. git, and you want to fast iterate on the part without the requirement to check in to a repo (even one on your local file system). It was previously possible to override the source-type to local by putting any non-valid value in your local snapcraft.yaml source-type field whilst keeping all the other useful stuff like build-packages etc." |
|
Ah, we certainly need a unit test at least for this so we do not regress again. |
|
@EduardoVega this is related to the code you touched. If you have some spare minutes, would you mind reviewing it after @jocave adds the test? |
|
Hi @ElOpio , @sergiusens and @jocave . The change looks good. In fact it makes sense to have the |
Allows local overriding of a source-type to local LP: #1695886
jocave
force-pushed
the
allow-source-type-local
branch
from
42b52be
to
848fd14
Compare
| @@ -60,3 +61,7 @@ def test_extract_and_keep_debfile(self): | |||
|
|
|||
| with open(deb_download, 'r') as deb_file: | |||
| self.assertEqual('Test fake compressed file', deb_file.read()) | |||
|
|
|||
| def test_has_source_handler_entry_on_linux(self): | |||
| if sys.platform == 'linux': | |||
There was a problem hiding this comment.
I find it nicer to use skip unless: https://docs.python.org/3/library/unittest.html#unittest.skipUnless
| if sys.platform == 'linux': | ||
| self.assertTrue(sources._source_handler['rpm'] is sources.Rpm) | ||
| else: | ||
| self.assertRaises(KeyError, sources._source_handler['rpm']) |
There was a problem hiding this comment.
You are doing it differently here than in the deb case. Any reason?
There was a problem hiding this comment.
No, I meant for both to be like the rpm case. Opened atom to find not all files had been saved though!
There was a problem hiding this comment.
@jocave your tests feel a little weird, because they are just checking that a dict is properly filled. That doesn't hurt, but it also doesn't provide a lot of value. I was thinking more about something like testing a pull, with source: local in the yaml. We don't seem to have any unit tests like this, which is something we need to change. So what do you think about an integration test instead?
|
@ElOpio sure, I can add an integration test. |
| version: 0.1 | ||
| summary: one line summary | ||
| description: a longer description | ||
| icon: icon.png |
There was a problem hiding this comment.
Please toast the icon, it's not necessary anymore (yeah I know, some of our older integration tests still have them, not your fault).
There was a problem hiding this comment.
@kyrofa no problem, it's done...
Allows local overriding of a source-type to local
LP: #1695886