search

gjcarneiro / yacron

A modern Cron replacement that is Docker-friendly
MIT License
449 stars 38 forks source link

Add `env_file` Job configuration option #43

Closed thatsed closed 3 years ago

thatsed commented 3 years ago

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 commented 3 years ago

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...

thatsed commented 3 years ago

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 commented 3 years ago

Great, thanks! :+1: