DocNow / diffengine

track changes to the news, where news is anything with an RSS feed
MIT License
178 stars 30 forks source link

Environment vars in `config.yaml` file #67

Closed nahuelhds closed 4 years ago

nahuelhds commented 4 years ago

Target: to keep my credentails and tokens secretly so I can use them safely in services like Heroku, Vercel or any other.

Usage

The configuration file

In the config.yaml file

twitter:
  consumer_key: "${MY_CONSUMER_KEY_ENV_VAR}"
  consumer_secret: "${MY_CONSUMER_SECRET_ENV_VAR}"

Defining the env vars

Every service like Heroku, Vercel, etc, offers a way to define environment vars for the project. Just define them there.

Othewsie, you can define them locally (or remotely too if you have your own server), by creating a .env file, this way:

MY_CONSUMER_KEY_ENV_VAR=CONSUMER_KEY
MY_CONSUMER_SECRET_ENV_VAR=CONSUMER_SECRET

Of course, this file has to be ignored in the version control.

edsu commented 4 years ago

Looks good! So it looks like you would rather submit discrete pull requests for new functionality instead of working with the [threads] branch that merged in your entire master? That is fine with me. I just thought it could be easier to shape what you have on master since you have added quite a few new things.

nahuelhds commented 4 years ago

Yes, I really prefer this approach so I can make the proper tests and refactor the code properly!

nahuelhds commented 4 years ago

I've also created an issue to see if it's possible to define a common formatter for the entire project. So when I pull changes they really only focus on the what actually changed and not about formatting diffs (like happened here with some whitespaces)

edsu commented 4 years ago

After I merged this I noticed that the test is failing, and was wondering if you are seeing the same?


FAILED test_diffengine.py::test_environment_vars_in_config_file - TypeError: home_path() takes 1 posit...
==========
E       TypeError: home_path() takes 1 positional argument but 2 were given
edsu commented 4 years ago

I'll see if I can fix this, which will help me understand the changes better!

edsu commented 4 years ago

@nahuelhds make sure you merge the latest from master for this change to get the tests working again.

edsu commented 4 years ago

Hmm, the test works for me locally, but not in Travis. Maybe Travis already uses .env? Maybe we can skip this test on Travis...

edsu commented 4 years ago

I marked the test for skipping when running under Travis.

nahuelhds commented 4 years ago

Oh you use Travis. Wouldn't be nice to connect Travis to your master repo so we could wait for the CI to pass and avoid this kind of issue, don't you think?

Regard the issue itself, I'm seeing now that fails for me locally but don't worry. I'll try to make it work by using another env file name so it doens't mess with the original one from Travis

edsu commented 4 years ago

Yes, Travis should only run on the master branch. I just pushed a change for that to Master. I'll be more careful about verifying new tests work before merging in the future!