-
Notifications
You must be signed in to change notification settings - Fork 4
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
Initial extraction from InfluxData #2
Initial extraction from InfluxData #2
Conversation
- Generate with `mix new . --app config_cat` - Don't overwrite README or .gitignore - Append the README contents that would have been generated to the existing README
This commit contains the code almost exactly as it has been running in production at InfluxData. Changes here: - Add necessary dependencies to mix.exs - Replace use of `Ecto.UUID.generate/0` with `UUID.uuid4/0` to avoid depending on Ecto - Configure the log level in `test_helper.exs` - Add `@tag capture_log: true` to noisy tests. Co-authored-by: Iris Scholten <[email protected]> Co-authored-by: Delmer Reed <[email protected]> Co-authored-by: Marc Delagrammatikas <[email protected]> Co-authored-by: Mansi Gandhi <[email protected]> Co-authored-by: Kristin Albright <[email protected]> Co-authored-by: Nick Stalter <[email protected]>
FYI: I've created a branch called |
@randycoulman The fact you are working on a fork means I can't push to the same PR as you and we can't collaborate on the same PR. So I pushed your changes (along with some of mine) to this branch instead: |
Or, if you prefer, you can try to address the changes @laliconfigcat suggested and I can jump in later. |
@laliconfigcat I've gone through all of your comments and extracted them to new issues. Now that we're on a 1.0 branch, can we go ahead and merge this as-is and then start addressing the issues in separate PRs? This is already a pretty big PR and I think it'll be easier to review the various changes separately. Thanks! |
Closes #1
This contains the code almost exactly as it has been running in production at InfluxData. We're open to make any changes or improvements necessary to get this in good shape for a wider community.
Generated initial app skeleton:
mix new . --app config_cat
Extract library from InfluxData's app. I made the following changes to what we have running:
Ecto.UUID.generate/0
withUUID.uuid4/0
to avoid depending on Ecto.test_helper.exs
(we had it in application config, but that's not recommended for libraries).@tag capture_log: true
to noisy tests. See note below.NOTES:
I'm using Elixir 1.10.4; I tried using 1.11, but we get a warning from the
elixir_uuid
library regarding its use of the:crypto
library. Elixir 1.11 does some extra checking of this sort. This is an open PR to address this: Adds :crypto as a dependency in extra_applications zyro/elixir-uuid#47. We'll ultimately need to decide which Elixir versions we want to support and configure those in the Travis build matrix.I added
mix_test_watch
as a dev-only dependency. It's very handy to be able to runmix test.watch --stale
while developing so that tests run automatically as you make changes.I attempted to add some tests around the log output, but because the logging comes from a separate process, it's harder to capture that way. If this is something we want tests around, we can do that as separate PR.