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

feat: Deduplicate config #20

Merged
merged 1 commit into from
Aug 10, 2022
Merged

feat: Deduplicate config #20

merged 1 commit into from
Aug 10, 2022

Conversation

whuang1202
Copy link
Contributor

Deduplicating configuration into config file
The config file is mostly the same as the common-config given, null checks are added in to the get_producer function in order to accomodate these functions on a separate file.

@timmc-edx when you wrote the override_settings in the test file did you intend for the name of the settings (SCHEMA_REGISTRY_URL for example) to not have EVENT_BUS_KAFKA before it? I couldn't make the tests run as intended without the EVENT_BUS_KAFKA in front of it.

@whuang1202 whuang1202 requested a review from timmc-edx August 8, 2022 19:19
@timmc-edx
Copy link
Contributor

timmc-edx commented Aug 8, 2022

Yeah, in my proof-of-concept branch I had changed the setting names in the new code, but hadn't updated the tests to match. The first part of that was intentional, though. :-)

This will also want a CHANGELOG.rst entry and a major-version bump (in the root module's __init__.py).

@timmc-edx timmc-edx self-assigned this Aug 8, 2022
@whuang1202 whuang1202 changed the title Common config perf: common config dedpulication Aug 9, 2022
@whuang1202 whuang1202 force-pushed the common-config branch 6 times, most recently from 347f6ec to 07b534a Compare August 10, 2022 17:02
Configuration is put in config.py rather than inside the send to event bus function so it can be more widely used,
do note that setting names are changed to have the EVENT_BUS_KAFKA prefix for use.
@whuang1202 whuang1202 changed the title perf: common config dedpulication feat: Deduplicate config Aug 10, 2022
@whuang1202 whuang1202 merged commit a8ee8de into main Aug 10, 2022
@whuang1202 whuang1202 deleted the common-config branch August 10, 2022 17:47
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