nervous-systems / cljs-lambda

Utilities around deploying Clojurescript functions to AWS Lambda
The Unlicense
310 stars 34 forks source link

Add support for "new" Lambda env variables #56

Closed zrzka closed 7 years ago

zrzka commented 7 years ago

Documentation here.

Example:

:cljs-lambda {
  :defaults {:env {:table-name "abc"}}
  :functions [{
    :name "abc"
    :invoke "def"
    :env {:extra-stuff "xyz"}}]}

:defaults contains env variables for all functions. :env inside :functions list is merged with environment variables from :defaults.

Docs quote: environment variables must start with a-zA-Z and can contain a-ZA-Z0-9_.

We will transform keywords in this way:

We will not check these variables and we will let aws cli to fail if they do not meet requirements.

We need to add this to:

Update function is little bit harder, because we have to get function configuration (if it exists) and compare it. We can do it in the same way as we did in https://github.com/nervous-systems/cljs-lambda/pull/48.

Sounds good?

zrzka commented 7 years ago

P.S. I'll skip KMS key in this round, because the default one is used automatically and is free of charge.

moea commented 7 years ago

@zrzka I think this proposal makes sense, the only snag is whether/how to integrate it with the existing environment variable support, which is basically a hack which sets the variables for all functions within the Node process, before the handler is invoked. I think abandoning that and using the AWS support for per-function environment variables is the right approach. The one thing I like about the existing feature is the :capture functionality for copying values from the build environment by partially/exactly matching the name.

My concern with mangling is that the keys will likely be looked up at runtime in an environment which doesn't have access to the same keyword->env var mangling function. The runtime library has a facility for retrieving environment variables which is provided so that the environment can be mocked during tests - if we went with mangling, that function could be modified to perform the same transformations before doing a lookup - but I'm sure not many people use that function. I personally prefer unmangled values for those reasons, but I'm open-minded.

My proposal would be allowing strings or keywords, but not mangling, removing the existing environment variable setting, but supporting a similar format which continues to allow build-time capture:

:cljs-lambda {
  :defaults {:env {:set {"TABLE_NAME" "abc"}
                   :capture ["EXACT_MATCH" #"PARTIAL_"]}}
  :functions [{
    :name "abc"
    :invoke "def"
    :env {:set {:EXTRA_STUFF "xyz"}}}]} 

Does any of that make sense?

If you think capturing is going to happen a lot less often than setting fresh variables, an alternative format would be {:env {"TABLE_NAME" "abc" :cljs-lambda/capture ["EXACT_MATCH" ...]}} - i.e. an optional namespaced key, which would allow the :set to be avoided in the common case.

zrzka commented 7 years ago

@moea I agree with mangling, we should skip it. Not sure about :capture, because I don't use it and don't know anyone who actually use it as well. But we can keep it there and do it your way:

project.clj

:cljs-lambda {
  :defaults {:env {:set {"TABLE_NAME" "abc"}
                   :capture ["EXACT_MATCH" #"PARTIAL_"]}}
  :functions [{
    :env {:set {:EXTRA_STUFF "xyz"}
          :capture ["ANOTHER_MATCH"]}}]} 

profiles.clj

{:production {:cljs-lambda {:defaults {:env {:set "TABLE_NAME" "def"}
                                             :capture ["EXACT_MATCH" #"PARTIAL_"]}}}}

Processing

moea commented 7 years ago

@zrzka Yeah, that sounds good. I just checked, I'm not using :capture - if it's awkward to reuse the existing code, or you don't like the feature, then removing it is fine - I think the user could interpolate the variables themselves, anyway, in the project file anyway, like {"TABLE_NAME" ~(System/getenv "TABLE_NAME")}

zrzka commented 7 years ago

@moea it's not awkward, it just looks complicated, maybe confusing, ... Compare ...

{:defaults {:env {"TABLE_A" "table"
                  "TABLE_B" ~(System/getenv "TABLE_B")}}}

vs

{:defaults {:env {:set {"TABLE_A" "table"}}
                  :capture ["TABLE_B"]}}

First one looks more cleaner.

And now imagine how complicated it can look like when leiningen starts merging (iow replacing) lists of captures when additional profiles are provided, what has higher / lower precedence, ...

I don't know, I'm tempted to propose :capture removal and use simple :env without :set and :capture.

moea commented 7 years ago

@zrzka Cool, that's totally fine with me.

zrzka commented 7 years ago

@moea okay, cool, will do it

zrzka commented 7 years ago

Didn't forget ;-) We have a release next week, so, then will have time for this. And will add DLQ configuration as well. Sry for the delay.

moea commented 7 years ago

@zrzka Excellent!

zrzka commented 7 years ago

Okay, so, to sum it up after a while ... Configuration example ...

:cljs-lambda {:defaults  {:role "arn:aws:..."
                          :dead-letter "arn:aws:..."
                          :env {"TABLE_A" "aaaa"
                                "TABLE_B" ~(System/getenv "TABLE_B")}}
              :functions [{:name "SomeLambdaFunctionName"
                           :invoke some.lambda.function.name/handler
                           :dead-letter "arn:aws:..."
                           :env {"TABLE_C" "ccccc"}}]}

AWS CLI documentation:

moea commented 7 years ago

@zrzka Looks good to me

zrzka commented 7 years ago

Implemented & merged via following PRs:

Available since 0.6.5-SNAPSHOT. Closing.