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.

Sign up for GitHub

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

Jump to bottom

[DPE-3721] - chore: use tools-log4j.properties for run_bin_command #201

Merged
merged 9 commits into from
May 30, 2024
Merged

[DPE-3721] - chore: use tools-log4j.properties for run_bin_command #201

merged 9 commits into from
May 30, 2024

Conversation

marcoppenheimer
Copy link
Contributor

@marcoppenheimer marcoppenheimer commented May 28, 2024 •
edited
Loading

Changes Made

chore: pass tools-log4j.properties to run_bin_command

refactor: minor tidy-ups

  • Renaming KafkaConfigManager --> ConfigManager. The 'Kafka' part seemed redundant
  • JAAS indentation was wonky with multiline strings, now it's aligned in a sensible way

@marcoppenheimer marcoppenheimer changed the title [DPE-3721] - refactor: rename class to ConfigManager [DPE-3721] - chore: use tools-log4j properties for run_bin_command May 28, 2024
@marcoppenheimer marcoppenheimer changed the title [DPE-3721] - chore: use tools-log4j properties for run_bin_command [DPE-3721] - chore: use tools-log4j.properties for run_bin_command May 28, 2024
@marcoppenheimer marcoppenheimer mentioned this pull request May 28, 2024
Merged
deusebio
deusebio approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

src/managers/config.py Show resolved Hide resolved
Batalex
Batalex approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@Batalex Batalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
We could have kept KafkaConfigManager, since we might have a CruiseControlManager coming soon (and maybe a kraft one if the config differs enough), but this won't change much comprehension-wise

src/managers/config.py Show resolved Hide resolved
zmraul
zmraul approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I agree with Alex though, config managers will probably get reverted/changed soon.

@marcoppenheimer marcoppenheimer merged commit a5d7828 into main May 30, 2024
16 checks passed
@marcoppenheimer marcoppenheimer deleted the chore/use_kafkalog4jopts branch May 30, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmraul zmraul zmraul approved these changes

@Batalex Batalex Batalex approved these changes

@deusebio deusebio deusebio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcoppenheimer @deusebio @Batalex @zmraul