arabold / serverless-export-env

Serverless plugin to export environment variables into a .env file
MIT License
102 stars 34 forks source link

v2.0.0-alpha.0 thoughts #33

Closed serverlesspolska closed 3 years ago

serverlesspolska commented 3 years ago

Hi @arabold,

It's awesome that you're working on this plugin!

I have read the alpha release notes and have a question regarding:

Running sls export-env will no longer merge the environment variables of all functions into a single .env file. Instead pass the name of the desired function as --function argument to the command line. If no function is specified, only global environment variables will be exported.

What's the reason behind it?

In my use cases I need all environment variables in a single file, that simplifies a lot for me. Is it possible to request a --all parameter to have it backward compatible with current versions behavior?

arabold commented 3 years ago

Hi @serverlesspolska , thanks for the feedback.

The reason is that functions can define different, conflicting environment variables. Like this example:

provider:
  environment:
    MY_ENV_VAR: foo

functions:
  hello1:
    handler: ./hello1.handler
    environment:
      MY_ENV_VAR: hello1
  hello2:
    handler: ./hello2.handler
    environment:
      MY_ENV_VAR: hello2

What would be the expected value of MY_ENV_VAR? foo, hello1 or hello2? The behavior wasn't really predictable and thus I decided to remove it for now. I (wrongly?) assumed that shared environment variables would be listed in provider.environment anway.

Having that said, I'm totally open to add a --all (and an equivalent serverless.yml setting). What would be the expected behavior for conflicts like these? Any suggestions?

arabold commented 3 years ago

@serverlesspolska , try 2.0.0-alpha.1 😉

serverlesspolska commented 3 years ago

Hi @arabold,

Thanks for meeting my request. 🙏

To be honest, this case didn't come to my mind because I name my env variables carefully. Even when two lambda functions have the same variables they always have the same value (if not then it was my mistake). But now I see that this is probably not the case for everyone (even though could be considered as a bad practice 😉 )

Having that said, I'm totally open to add a --all (and an equivalent serverless.yml setting). What would be the expected behavior for conflicts like these? Any suggestions?

Based on what I wrote above I don't care, could be first / last / random 🙂 After some thinking random could be the best option here. I will explain it later.

I have tested 2.0.0-alpha.1 with --all and it works superb. Thanks a lot. I even added the same env variables to the second function with different value (the case you have described) and value from the 2nd (last functions defined in serverless.yml) was used.

I use your plugin for automated tests. I inject a file generated by your plugin into jest testing context. Thus, I can refer to env variables in my local test. That's why I want to have all values in single files (I don't need to change config whenever I add new Lambda to the project).

Since I use it for testing, random value would be the best since at some point when running the test it should fail, and that would give me the feedback that something is wrong.

If you want to see how this testing works checkout my Severless Framework template (still uses old version of your plugin) at https://github.com/serverlesspolska/serverless-hexagonal-template

williamsandonz commented 3 years ago

Hey @arabold. I hope you don't mind me also sharing some feedback on v2. I've checked it out locally and had a play and it looks great! One thing I've noticed is that the GetAtt solution seems to be quite limited and I've been struggling to make it work for my use-case. For example, when trying to access Fn::GetAtt: [RdsPostGres, Endpoint.Address] whereby RdsPostGres is a AWS::RDS::DBInstance it won't resolve. I noticed in resolveGetAtt() the arguments don't provide sufficient information to retrieve .Endpoint.Address and in the code your explicitly adding entries into the switch statement. I feel like this is insufficient for a few reasons:

  1. It's not possible to resolve a value that requires information other than PhysicalResourceId (or any other properties from 'resource' argument). Which I think will be quite a common scenario and people will be forced to add mocks in (E.G Endpoint.Address, I'm sure there are many others).
  2. Even for these cases that can theoretically be resolved, an explicit case in the switch will need to be added for each one.

I'm wondering if there is a generic function that could be added to aws-helpers.js that calls AWS.request("CloudFormation", "X) which fetches a list of all attributes for a resource so that the code in resolveGetAtt() could simply search that list.

That way all use-cases for GetAtt would always resolve...What do you think? Happy to help try find a solution for this.

arabold commented 3 years ago

Hi @williamsandonz . You're absolutely right that the handling of Fn::GetAtt is lacking. I've looked into ways of somehow resolving attributes via the CloudFormation API, but I haven't found anything so far. At this point, I'm not sure if it even makes sense to try it in the first place, and I'm considering removing the resolution code again...

Having that said, there're two workarounds:

1. The getAttMap configuration option:

custom:
  export-env:
    getAttMap:
      RdsPostGres:
        Endpoint.Address: "xxxxx.amazonaws.com"

So, in this case, you have to hard-code the resolution in the serverless.yml. This isn't a pretty one, but it should work for local testing and similar use cases.

2. Export the value

Instead of accessing the resource directly in your environment variable definition, try exporting and re-importing it instead. This has the advantage that the resolution is done by AWS during deployment and you only have to import the resolved value again. Something like this should work:

environment:
  RDS_ENDPOINT_ADDRESS: !ImportValue RdsEndpointAddress

resources:
  Resources:
    # ...
  Outputs:
    RdsEndpointAddress:
      Value: !GetAtt [RdsPostGres, Endpoint.Address]
      Export:
        name: RdsEndpointAddress

I'm not perfectly sure if this will work in the same serverless.yml as the export might need to exist before the environment variable can be resolved, but it might be worth a try. You might possibly have to deploy it without the environment variable first and then add it in a second step after the export has already been created :(

If you try any of the above, let me know how it went. I'll update the docs accordingly, too, to help others running in similar issues.

williamsandonz commented 3 years ago

Hi @arabold . Ah I can see what you mean, I assumed AWS offered an endpoint for that but looks like it doesn't exist, that's such a shame! Indeed, could be worth seeing if the Import/Export approach is viable, seems odd to refer to a stack output as input for lambda but if it is deployed after resources created then maybe it will work! I like the mocking idea, having the ability to prevent [object Object] reaching the .env file is definitely needed and likewise for these values reaching lambda at runtime when running with sls offline.

williamsandonz commented 3 years ago

Just thinking, there is one other alternative. Using RDS as an example, you could use DescribeStackResource to get the physical ID then use https://awscli.amazonaws.com/v2/documentation/api/latest/reference/rds/describe-db-instances.html to get additional information. So for each GetAtt you'd need to find the Resource and figure out it's type then call a specific endpoint just for that resource to get the additional info. Would be a fair bit of work but fairly robust.

arabold commented 3 years ago

I've pushed 2.0.0 now. Any custom logic for Fn::GetAtt is disabled for now. We'll have to rely on the getAttMap configuration option instead. It would be great if there's a way of resolving Fn::GetAtt automatically at some point, but it seems we would have to implement a custom resolution for every single resource type. 😞