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

feat: Support list of env files #506

Merged
merged 11 commits into from
Feb 22, 2024
Merged

feat: Support list of env files #506

merged 11 commits into from
Feb 22, 2024

Conversation

LeonLuttenberger
Copy link
Contributor

Issue

#500

Description of changes

Add ability to specify multiple env files:

seedfarmer apply addf --env-file .env --env-file .env1 --env-file .env2

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@LeonLuttenberger LeonLuttenberger added the enhancement New feature or request label Feb 22, 2024
@LeonLuttenberger LeonLuttenberger self-assigned this Feb 22, 2024
Copy link
Contributor

@dgraeber dgraeber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sooo...there are multiple places the env is used (in the CLI) - all are addressed here?

Is this backward compatible (just want to confirm)?

Did you test if ordering is preserved (I was wondering this myself)?

I suggest adding a _logger.debug to write out to the console the final resolution of all env parameters

@LeonLuttenberger
Copy link
Contributor Author

LeonLuttenberger commented Feb 22, 2024

Sooo...there are multiple places the env is used (in the CLI) - all are addressed here?

I did a search for references to --env-file in the codebase, and I changed all the ones I found. So unless there are places where the environment parameter is named differently, this should be everything

Is this backward compatible (i.e customers are passing in unbracked strings...does this work for both)?

This isn't allowing the passage of bracketed strings. Instead, it's making use of click's option to allow multiple values of a parameter. So --env-file "[.env, .env2]" won't work, but --env-file .env --env-file .env2. Another benefit of this approach is that it's automatically backwards compatible, since it doesn't require a different way for each env-file parameter to be defined.

Did you test if ordering is preserved (I was wondering this myself)?

Yes. If .env1 and .env2 are passed in that order, values from .env2 will override values from .env1 with the same key. I would like to find a more robust way of testing this though. The issue is that the test_command function execution everything in isolation, so I can't check the env variables in my unit test.

I suggest adding a _logger.debug to write out to the console the final resolution of all parameters

I like that idea.

@dgraeber
Copy link
Contributor

Good work!! Also...update seed-farmer/docs/source/manifests.md near the (envVariable) section....just say that you can pass in multiple files in the order they should be resolved.

@dgraeber dgraeber merged commit 52661f6 into awslabs:main Feb 22, 2024
2 checks passed
@LeonLuttenberger LeonLuttenberger deleted the feat/list-of-env-params branch February 22, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants