Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set env vars from config.json when running chalice local #411

Merged
merged 2 commits into from
Jul 25, 2017

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Jul 11, 2017

This builds on #397 and pulls up setting up the env vars up to cli/init.py. We may need to pull this up even higher because we need to make sure env vars are set before ever importing the chalice app. Otherwise we'll crash with KeyErrors on import is module level code accesses os.environ.

cc @stealthycoin @kyleknap

Steven Braverman and others added 2 commits July 11, 2017 17:50
The env var configuration needs to be set before importing
the app so that any module level code will be able to access
env vars.

We might need to pull this up even higher to the cli() function
because any time we import the chalice app we need to make sure
env vars are set.  For now this is just in the 'local' command.
@codecov-io
Copy link

codecov-io commented Jul 11, 2017

Codecov Report

Merging #411 into master will increase coverage by 0.22%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   93.54%   93.77%   +0.22%     
==========================================
  Files          18       18              
  Lines        2805     2809       +4     
  Branches      369      369              
==========================================
+ Hits         2624     2634      +10     
+ Misses        133      127       -6     
  Partials       48       48
Impacted Files Coverage Δ
chalice/cli/__init__.py 81.76% <100%> (+3.99%) ⬆️
chalice/cli/factory.py 89.1% <66.66%> (-0.69%) ⬇️

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 c458a5c...d49bf39. Read the comment docs.

@sbraverman
Copy link

@jamesls thanks for picking this up and running with it. Overall updates look good. We can close my original PR in favor of this one.
I have a couple questions:

  • What is the benefit of passing os.environ into run_local_server as opposed to just using os.environ directly inside of run_local_server?
    i.e. os.environ.update(config.environment_variables) inside of run_local_server

  • One thing I was trying to do in the original PR was ensure all environment_variables were casted to Strings.
    Running the current code with this change currently does...

(venv-chalice) Braverman@local:~/Repos/fix-local-env-vars $ chalice local
must be string, not bool

Do you think:

  • another validation check should be added to _validate_config_from_disk to ensure proper strings are used for environment_variables
  • environment variable values should get casted to Strings automatically

Let me know if that makes sense or more clarification is necessary. Thanks again for looking into this.

@jamesls
Copy link
Member Author

jamesls commented Jul 25, 2017

Hi, sorry for the delay, I've been on vacation for a few weeks. To answer your questions:

  1. Primarily just to make testing easier. Not a huge deal but I prefer the existing way so you pass in your own os.environ as well
  2. For validation, I think raising an error would make sense here. Here's what you currently get if you use an invalid type:
$ chalice deploy
Updating IAM policy.
Updating lambda function...
Regen deployment package...
Updating IAM policy.
Sending changes to lambda.
Parameter validation failed:
Invalid type for parameter Environment.Variables.foo, value: 5, type: <type 'int'>, valid types: <type 'basestring'>

Let me know if you have any more questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants