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

Add env_file Job configuration option #43

Merged
merged 4 commits into from
Mar 14, 2021

Conversation

thatsed
Copy link
Contributor

@thatsed thatsed commented Feb 26, 2021

Hi, I've added an env_file option in the Job configuration to load environment variables from file.

The implementation is standalone (no external libraries such as dotenv) and only supports parsing comments (Lines starting with #), empty lines and space indentation.

@gjcarneiro
Copy link
Owner

Apologies for lack of feedback. This PR is not bad at all, but it has no tests, so it will require more work (from either of us) before it can be merged.

It's in a similar situation as my own PR #40: I wrote the code, wasn't hard, but I haven't yet found time and motivation for writing corresponding tests. But tests are important, so...

Copy link
Owner

@gjcarneiro gjcarneiro left a comment

Choose a reason for hiding this comment

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

Some minor changes nitpicking. In addition, this needs some sort of test. And finally please format your code with black.

yacron/config.py Outdated Show resolved Hide resolved
yacron/config.py Outdated Show resolved Hide resolved
* moved file parsing in external helper function
* moved environment merging to JobConfig constructor
* added unit tests and fixtures
@thatsed
Copy link
Contributor Author

thatsed commented Mar 8, 2021

Apologies for lack of feedback. This PR is not bad at all, but it has no tests, so it will require more work (from either of us) before it can be merged.

It's in a similar situation as my own PR #40: I wrote the code, wasn't hard, but I haven't yet found time and motivation for writing corresponding tests. But tests are important, so...

I did write some tests, but decided against committing them due to not knowing where to put the fixtures (test env files). I've just moved them in a fixtures directory, please let me know if you find them out of place.

Edit: I did not commit them though - that's resolved. Thank you for taking the time to make an accurate code review!

@gjcarneiro gjcarneiro merged commit d664ae7 into gjcarneiro:master Mar 14, 2021
@gjcarneiro
Copy link
Owner

Great, thanks! 👍

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.

2 participants