/snapcraft

adt fixes for maven and noninteractive deb install #305

Merged
merged 1 commit into from Feb 10, 2016

Conversation

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

sergiusens commented Feb 5, 2016

This makes sure we setup noninteractive for deb installation and
also sets up the proxy configuration for maven.

LP: #1542511

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
examples_tests/tests.py
+ ' <nonProxyHosts>.internal</nonProxyHosts>\n' + \
+ ' </proxy>\n' + \
+ ' </proxies>\n' + \
+ '</settings>\n'
@elopio

elopio Feb 6, 2016

Member

If you put this string inside parentheses, you don't need the \ nor the +, which I find really useful.
There's also the option of using three quotes and textwrap.dedent, which I don't like that much but it's common.

examples_tests/tests.py
+ proxy = urlparse(os.environ['http_proxy'])
+ if not os.path.exists(os.path.dirname(settings_path)):
+ cls.addCleanup(os.rmdir, os.path.dirname(settings_path))
+ os.makedirs(os.path.dirname(settings_path), exist_ok=True)
@elopio

elopio Feb 6, 2016

Member

You are calling dir name three times . I would put it in a var.

integration_tests/test_maven_plugin.py
+ os.makedirs(os.path.dirname(settings_path), exist_ok=True)
+ with open(settings_path, 'w') as f:
+ f.write(_MVN_SETTINGS.format(proxy.hostname, proxy.port))
+ self.addCleanup(os.remove, settings_path)
@elopio

elopio Feb 6, 2016

Member

We can start moving common helpers to snapcraft/tests/utils, or you can turn this into a fixture and put it in snapcraft/tests/fixture_setup.
I think we shouldn't duplicate things, even if they are in different suites. That makes the suite rely on the snapcraft library, not just the snapcraft binary, but that's ok for me. What do you think?

@elopio
Member

elopio commented Feb 6, 2016

Thanks a lot for working on this, because I made no useful progress...
@kyrofa
Member

kyrofa commented Feb 10, 2016

Looks good to me! 👍
examples_tests/tests.py
@@ -16,6 +16,7 @@
import logging
import os
+import os.path
@elopio

elopio Feb 10, 2016

Member

There's no need to import os.path. By importing os you can just use it.

@sergiusens

sergiusens Feb 10, 2016

Collaborator

fixed

snapcraft/plugins/maven.py
@@ -42,6 +43,26 @@
logger = logging.getLogger(__name__)
+_MVN_SETTINGS = (
@elopio

elopio Feb 10, 2016

Member

A nit, I find it useful to suffix the format constants so it's easier to know that they need an argument.
Like _MVN_SETTINGS_FORMAT.

@sergiusens

sergiusens Feb 10, 2016

Collaborator

fixed

snapcraft/tests/test_plugin_maven.py
+ plugin = maven.MavenPlugin('test-part', self.options)
+ os.makedirs(plugin.sourcedir)
+ glob_mock.return_value = [
+ os.path.join(plugin.builddir, 'target', 'fake')]
@elopio

elopio Feb 10, 2016

Member

You are not using this value in the test, it seems it's just needed to be able to test the the call to run. To make that clear I usually call the value 'dummy' instead of 'fake'.

@sergiusens

sergiusens Feb 10, 2016

Collaborator

fixed

snapcraft/tests/test_plugin_maven.py
+ settings_path = os.path.join(plugin.partdir, 'm2', 'settings.xml')
+ os.makedirs(plugin.sourcedir)
+ glob_mock.return_value = [
+ os.path.join(plugin.builddir, 'target', 'fake')]
@elopio

elopio Feb 10, 2016

Member

Same s/fake/dummy.

@sergiusens

sergiusens Feb 10, 2016

Collaborator

fixed

snapcraft/plugins/maven.py
+
+def _get_no_proxy_string():
+ no_proxy = [k.strip() for k in
+ os.environ.get('no_proxy', 'localhost').split(',')]
@elopio

elopio Feb 10, 2016

Member

You are missing tests for when no_proxy has one value, and when it has multiple values.

@sergiusens

sergiusens Feb 10, 2016

Collaborator

fixed

@elopio
Member

elopio commented Feb 10, 2016

Thanks for adding this. I'm 👍 with a couple of ignorable comments and a note about a missing test.
Also repo is severely undertested. I'll move it to the top of my pending coverage tasks.
snapcraft/tests/test_plugin_maven.py
+ self.addCleanup(os.environ.update, env_copy)
+
+ os.environ['SNAPCRAFT_SETUP_PROXIES'] = '1'
+ os.environ['http_proxy'] = 'http://localhost:3132'
@elopio

elopio Feb 10, 2016

Member

Oh, one more. This will pollute the environ for the following tests.
Use self.useFixture(fixtures.EnvironmentVariable(var, value))

@sergiusens

sergiusens Feb 10, 2016

Collaborator

I'm using addCleanup, I don't mind trying this though

@sergiusens
adt fixes for maven and noninteractive deb install 
This makes sure we setup noninteractive for deb installation and
also sets up the proxy configuration for maven.

LP: #1542511

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

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.6%) to 93.773%
Details
8a9555c
@elopio
Member

elopio commented Feb 10, 2016

Tests passed and we have +0.6%. Merging...
You earned a QA ⭐️

elopio added a commit that referenced this pull request Feb 10, 2016

@elopio
Merge pull request #305 from sergiusens/bugfix/1542511/adt 
adt fixes for maven and noninteractive deb install
a99610b

@elopio elopio merged commit a99610b into snapcore:master Feb 10, 2016
2 checks passed

2 checks passed

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

@sergiusens sergiusens deleted the sergiusens:bugfix/1542511/adt branch Feb 10, 2016

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

@elopio
Merge pull request #305 from sergiusens/bugfix/1542511/adt 
adt fixes for maven and noninteractive deb install
c1cbd26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment