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

Configuration update #46

Merged
merged 3 commits into from
Jul 6, 2020
Merged

Configuration update #46

merged 3 commits into from
Jul 6, 2020

Conversation

elise-scx
Copy link
Contributor

@elise-scx elise-scx commented Jul 6, 2020

The uses of this library I've seen in our repos are compatible with the changes in this PR, and wouldn't require any code changes to update. But other users might have to make some small changes, so I'll release this as a new major version.

Changes:

  1. There were several different ways to initialize ConsumerConfig, and they behaved differently. E.g. accepting/not accepting implicits, and initializing the default kinesis client using a different method. So you could get unexpected behavior when switching from one method to another. And there were other inconsistencies that were more of a usability issue, e.g. some values could be passed as params, but to set others you needed to use a builder-style copy method. So I changed things around to make them more consistent: (1) Moved all initialization and default logic into only two methods, apply and fromConfig, which behave the same way (both accept implicits, both initialize default clients with the same functions). The only difference is one reads certain values from a config, while the other has those values as params. (2) You can set any ConsumerConfig values using the params of these methods, so we no longer need the "with..." builder-style methods. (3) Moved configuration file reading into a separate object to keep it from getting mixed in with other initialization logic.

  2. Changed the example configuration location in the readme (does not change behavior): Older configuration settings weren't added to the readme before, and need to be in a specific location. So some of our configurations ended up with two separate sections for the same consumer: one with the old settings, and one in the location that was copy-pasted from the readme for the new settings. So I just updated the example to include the old configs, and put the new ones in the same section so people won't end up with two different sections. (Two sections will still work, since the new configs can go anywhere and the location is passed manually.)

@mergify mergify bot merged commit 3b26fc5 into master Jul 6, 2020
@mergify mergify bot deleted the update-config-creator branch July 6, 2020 19:30
@elise-scx
Copy link
Contributor Author

Auto-merged by accidental mergify settings! I won't tag this as a release right away so people have a chance to look... I can revert the merge if there are any concerns.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants