Skip to content

Conversation

@randycoulman
Copy link
Collaborator

@randycoulman randycoulman commented Oct 16, 2020

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
    • Don't overwrite README or .gitignore.
    • Append the README contents that would have been generated to the existing README. They're not accurate until we publish to hex.pm, but I didn't want to lose the content.
  • Extract library from InfluxData's app. I made the following changes to what we have running:

    • 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 (we had it in application config, but that's not recommended for libraries).
    • Add @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 run mix 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.

Randy Coulman and others added 2 commits October 15, 2020 19:28
- 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 <ischolten.is@gmail.com>
Co-authored-by: Delmer Reed <delmer814+1@gmail.com>
Co-authored-by: Marc Delagrammatikas <delagrammatikas@gmail.com>
Co-authored-by: Mansi Gandhi <mansimg@gmail.com>
Co-authored-by: Kristin Albright <albright.kristin0@gmail.com>
Co-authored-by: Nick Stalter <nickstalter@gmail.com>
@igorescobar igorescobar changed the base branch from main to v1.0.0 October 16, 2020 13:18
@igorescobar
Copy link
Collaborator

igorescobar commented Oct 16, 2020

FYI: I've created a branch called v1.0.0 based on the main branch. Let's target all upcoming PRs to this one instead of the main branch. After we feel it's time we can merge the v1.0.0 branch to the main one. 👍

@igorescobar
Copy link
Collaborator

@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:
https://github.com/configcat/elixir-sdk/tree/pr/randycoulman/2

@igorescobar
Copy link
Collaborator

Or, if you prefer, you can try to address the changes @laliconfigcat suggested and I can jump in later.

@randycoulman
Copy link
Collaborator Author

@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!

@igorescobar igorescobar merged commit 4377a42 into configcat:v1.0.0 Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can we help?

3 participants