Fix environment variables being passed to subprocess while packaging #504

stealthycoin · 2017-08-30T16:53:08Z

Fixes #501
cc @kyleknap @jamesls @JordonPhillips @dstufft

codecov-io · 2017-08-30T18:31:19Z

Codecov Report

Merging #504 into master will increase coverage by 0.11%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   94.01%   94.12%   +0.11%     
==========================================
  Files          18       18              
  Lines        2941     2946       +5     
  Branches      380      381       +1     
==========================================
+ Hits         2765     2773       +8     
+ Misses        129      126       -3     
  Partials       47       47

Impacted Files	Coverage Δ
chalice/compat.py	`51.61% <ø> (+3.04%)`	⬆️
chalice/deploy/packager.py	`95.7% <100%> (+0.07%)`	⬆️
chalice/utils.py	`96.24% <100%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a36ad3f...5784fae. Read the comment docs.

kyleknap

Looks good to me. I just had some small suggestions.

kyleknap · 2017-08-31T16:08:00Z

chalice/utils.py

@@ -78,6 +78,10 @@ def create_zip_file(source_dir, outfile):
 class OSUtils(object):
    ZIP_DEFLATED = zipfile.ZIP_DEFLATED

+    def environ(self):
+        # type: () -> Dict[str,str]
+        return os.environ.copy()


Should we be caching the copy of the os.environ on the first call and then return the cached copy on subsequent calls? Mainly just wondering in the case that if we update the environement variables that gets returned, the update will not actually get returned again if OSUtils.environ gets called again.

Not needed for this particular use case but that is a reasonable expectation of someone consuming the OSUtils class in general so I will add it.

kyleknap · 2017-08-31T16:09:53Z

tests/functional/test_package.py

@@ -162,9 +163,17 @@ def osutils():


 @pytest.fixture
-def pip_runner():
+def empty_env_osutils():


Does it makes sense to subclass from OSUtils for the EmptyEnv class?

It won't really help or hurt anything in terms of the actual code, but it may make it clearer to someone reading the tests where this came from and what (partial) interface its trying to replace so I will update that.

kyleknap · 2017-08-31T16:11:10Z

tests/unit/deploy/test_packager.py

@@ -46,6 +46,26 @@ def pip_runner():


 @pytest.fixture
+def custom_env_osutils():
+    class CustomEnv(object):


Same thing here about subclassing from OSUtils.

kyleknap · 2017-08-31T16:16:59Z

tests/unit/deploy/test_packager.py

+
+@pytest.fixture
+def pip_runner_custom_env(custom_env_osutils):
+    def create_pip_runner(env):


So idea. What if instead of making a bunch of different pip runners, we just made a pip_runner_factory fixture? That way if we add anything more to the PipRunner() class, we do not have to add a new fixture for each of these.

Yes I mainly didn't want to change the interface used in every single test.

jamesls

Capturing offline feedback:

Remove usage of .copy() for env vars
Clean up compat.py if subprocess_python_base_environ isn't needed anymore.

Fixes aws#501

kyleknap

Looks good. Just a couple of comments about removing some potential dead test fixtures.

kyleknap · 2017-09-05T17:17:18Z

tests/unit/deploy/test_packager.py

+
+
+@pytest.fixture
+def pip_runner_custom_env(custom_env_osutils):


This can be removed now that it is no longer used.

kyleknap · 2017-09-05T17:19:55Z

chalice/utils.py

@@ -78,6 +79,10 @@ def create_zip_file(source_dir, outfile):
 class OSUtils(object):
    ZIP_DEFLATED = zipfile.ZIP_DEFLATED

+    def environ(self):
+        # type: () -> MutableMapping
+        return os.environ


Yeah that works too as long as we were not just returning back a copy each time.

kyleknap · 2017-09-05T17:21:18Z

tests/functional/test_package.py

+
+
+@pytest.fixture
+def pip_runner(empty_env_osutils):


Do we need the pip_runner fixture anymore after the refactor?

What's the alternative? The pip_runner fixture is used in numerous tests still. I'm not following.

I was referring to the fact that he switched out the use of pip_runner with pip_factory. I was not sure if the pip_runner fixture was being used anymore. When I did a quick check, I did not see it being used anywhere else in the test_package.py file

Almost all the tests in TestDependencyBuilder in the class below use this. Am I missing something?

Nevermind. I was looking at the tests/unit/deploy/test_packager.py file when I was going over the file. The tests/functional/test_package.py definitely needs it. Did not help that the same fixture was duplicated in two different test files instead of having it in the conftest.py.

#504 * stealthycoin-fix-path-passed-during-packaging: Remove unused fixture Add entry to CHANGELOG Fix environment variables being passed to subprocess while packaging

stealthycoin requested review from dstufft, jamesls, JordonPhillips and kyleknap August 30, 2017 16:53

stealthycoin force-pushed the fix-path-passed-during-packaging branch from 2b415a9 to e005df9 Compare August 31, 2017 06:35

dstufft approved these changes Aug 31, 2017

View reviewed changes

JordonPhillips approved these changes Aug 31, 2017

View reviewed changes

kyleknap reviewed Aug 31, 2017

View reviewed changes

jamesls reviewed Aug 31, 2017

View reviewed changes

Fix environment variables being passed to subprocess while packaging

5784fae

Fixes aws#501

stealthycoin force-pushed the fix-path-passed-during-packaging branch from e005df9 to 5784fae Compare September 1, 2017 19:37

jamesls approved these changes Sep 5, 2017

View reviewed changes

kyleknap reviewed Sep 5, 2017

View reviewed changes

jamesls merged commit 5784fae into aws:master Sep 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix environment variables being passed to subprocess while packaging #504

Fix environment variables being passed to subprocess while packaging #504

stealthycoin commented Aug 30, 2017

codecov-io commented Aug 30, 2017 •

edited

Loading

kyleknap left a comment

kyleknap Aug 31, 2017

stealthycoin Aug 31, 2017

kyleknap Aug 31, 2017

stealthycoin Aug 31, 2017

kyleknap Aug 31, 2017

kyleknap Aug 31, 2017

stealthycoin Aug 31, 2017

jamesls left a comment

kyleknap left a comment

kyleknap Sep 5, 2017

kyleknap Sep 5, 2017

kyleknap Sep 5, 2017

jamesls Sep 5, 2017

kyleknap Sep 5, 2017

jamesls Sep 5, 2017

kyleknap Sep 5, 2017



		@pytest.fixture
		def pip_runner_custom_env(custom_env_osutils):

Fix environment variables being passed to subprocess while packaging #504

Fix environment variables being passed to subprocess while packaging #504

Conversation

stealthycoin commented Aug 30, 2017

codecov-io commented Aug 30, 2017 • edited Loading

Codecov Report

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls left a comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 30, 2017 •

edited

Loading