Repository navigation
fix(cli): use correct consumer kind wording in consumer-offset set - #4620
SunilKumarPradhan wants to merge 5 commits into
Conversation
explain(), the error context and the success message in set_consumer_offset.rs always said "consumer", even for -k consumer-group or -k external-group. Add get_consumer_info(), mirroring get_consumer_offset.rs, and use it in all three places. Add unit tests for the explain message of each consumer kind and assert the wording in the external-group CLI integration test. Closes apache#4459
|
Note for maintainers: @SunilKumarPradhan also has #4490 open. New contributors are asked to keep one open PR at a time. See CONTRIBUTING.md. |
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
Hi @SunilKumarPradhan, please rewrite your initial message to follow the PR template (see PULL_REQUEST_TEMPLATE.md). |
|
/skill team-review-slim |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4620 +/- ##
=============================================
- Coverage 80.83% 71.33% -9.50%
Complexity 3107 3107
=============================================
Files 1467 1402 -65
Lines 356533 196286 -160247
Branches 300421 140510 -159911
=============================================
- Hits 288194 140027 -148167
+ Misses 61617 49897 -11720
+ Partials 6722 6362 -360
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: The diff fixes the set consumer-offset messages to name the consumer kind and adds unit and integration coverage for all three kinds. The only finding is a new wording helper that duplicates the existing get-command helper.
Counts: critical 0, warning 0, nit 0, simplification 1
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
| } | ||
| } | ||
|
|
||
| pub fn get_consumer_info(&self) -> String { |
There was a problem hiding this comment.
simplification: SetConsumerOffsetCmd::get_consumer_info copies the three-arm match in GetConsumerOffsetCmd::get_consumer_info (core/cli/src/commands/binary_consumer_offsets/get_consumer_offset.rs:52) word for word. Move one pub(crate) fn consumer_info(consumer: &Consumer) into binary_consumer_offsets/mod.rs and call it from both commands.
GetConsumerOffsetCmd and SetConsumerOffsetCmd each had their own get_consumer_info() with an identical three-arm match on ConsumerKind. Move the match into a consumer_info() helper in binary_consumer_offsets/mod.rs and have both commands call it. Addresses review feedback on apache#4620.
|
fixed the PR message . /ready |
SunilKumarPradhan
changed the title
hubcio
changed the title
SunilKumarPradhan
changed the title
Which issue does this PR address?
Closes #4459
Rationale
consumer-offset setnamed every consumer kind "consumer" in its output, even for-k consumer-groupand-k external-group, which is misleading when scripting or reading logs.What changed?
explain(), the error context, and the success message inset_consumer_offset.rsalways said "consumer" regardless ofConsumerKind. Added aconsumer_info()helper (shared withget_consumer_offset.rs, which had the same wording) inbinary_consumer_offsets/mod.rsthat picks the right wording per kind, and used it in all three places.Local Execution
cargo fmt --check -p iggy-cli,cargo clippy -p iggy-cli --all-targets -- -D warnings,cargo test -p iggy-cli)AI Usage
I used github copilot.