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

Initial extraction from InfluxData #2

Merged
merged 2 commits into from
Oct 16, 2020
Merged

Initial extraction from InfluxData #2

merged 2 commits into from
Oct 16, 2020

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 <[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]>
lib/config_cat/user.ex Show resolved Hide resolved
lib/config_cat/user.ex Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/config_cat.ex Show resolved Hide resolved
lib/config_cat.ex Show resolved Hide resolved
lib/config_cat/fetch_policy.ex Show resolved Hide resolved
lib/config_cat/fetch_policy.ex Show resolved Hide resolved
lib/config_cat.ex Show resolved Hide resolved
test/rollout_test.exs Show resolved Hide resolved
lib/config_cat.ex Show resolved Hide resolved
@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