Fix importing vendor lib during config creation. #350
Conversation
Codecov Report
@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 90.14% 90.15% +0.01%
==========================================
Files 18 18
Lines 2039 2042 +3
Branches 252 253 +1
==========================================
+ Hits 1838 1841 +3
Misses 147 147
Partials 54 54
Continue to review full report at Codecov.
|
| # For loading the config locally we must add the vendor directory to | ||
| # the path so it will be treated the same as if it were running on | ||
| # lambda. | ||
| vendor_dir = os.path.join(self.project_dir, 'vendor') |
There was a problem hiding this comment.
You should use the same logic as above and ensure we don't add this multiple times.
tests/functional/cli/test_factory.py
Outdated
| data = f.read() | ||
| f.seek(0, 0) | ||
| f.write('from vendedlib.utils import data_getter\n%s' % data) | ||
| config = clifactory.create_config_obj() |
There was a problem hiding this comment.
Is this right? I would expect us to try to do something with data_getter and it's return value.
tests/functional/cli/test_factory.py
Outdated
| data = f.read() | ||
| f.seek(0, 0) | ||
| f.write('from vendedlib import submodule\n%s' % data) | ||
| config = clifactory.create_config_obj() |
There was a problem hiding this comment.
So after looking at this again, I'd still prefer the test to be explicit. This is implicitly relying on the fact that if we can't import the module then this code will propagate some exception in create_config_obj. I'd rather be more explicit, ensure that not only do we not raise an exception but we are in fact able to access the thing we're importing. As an example:
# submodule.py:
SOME_VALUE = 'from-submodule'
# app.py
from vendedlib import submodule
app = Chalice(app_name='foo')
app.from_submodule = submodule.SOME_VALUE
# test
assert app.from_submodule == 'from-submodule'
Or something along those lines. It's just more explicit about successfully importing the vendored submodule
tests/functional/cli/test_factory.py
Outdated
| with open(os.path.join(vendedlib_dir, 'submodule.py'), 'a') as f: | ||
| f.write('CONST = "foo bar"\n') | ||
| app_py = os.path.join(clifactory.project_dir, 'app.py') | ||
| with open(app_py, 'r+') as f: |
There was a problem hiding this comment.
This doesn't need to block merging, but wouldn't just be simpler to open the file at the end and append from vendedlib import submodule\napp.imported_value = submodule.CONST? Not really sure if the .seek(0, 0) and .seek(0, 1) make the test any clearer.
|
Oh and don't forget to add an entry to the changelog, thanks. |
Adds the vendor directory to the path when loading the app locally. This is required for both deployment and local testing since both commands load the app.py file, which will fail if anything is imported from the vendor directory without it being added to the pythonpath. fixes aws#349
Also updated a test to make it a little more clear what it is testing and what it is not.
stealthycoin
force-pushed
the
fix-vendor-deploy
branch
from
6acfc24
to
4f51fa9
Compare
stealthycoin
force-pushed
the
fix-vendor-deploy
branch
from
4f51fa9
to
a9f3411
Compare
Adds the vendor directory to the path when loading the app locally. This
is required for both deployment and local testing since both commands
load the app.py file, which will fail if anything is imported from the
vendor directory without it being added to the pythonpath. fixes #349
I also manually tested
chalice deployandchalice localcc @jamesls @kyleknap