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

Added topic id to describeTopics Response #1068

Merged
merged 20 commits into from
Oct 21, 2023

Conversation

pranavrth
Copy link
Member

No description provided.

@cla-assistant
Copy link

cla-assistant bot commented Oct 2, 2023

CLA assistant check
All committers have signed the CLA.

@cla-assistant
Copy link

cla-assistant bot commented Oct 2, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

mostly just minor stuff

kafka/adminapi.go Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
{
"Brokers": "mybroker or $BROKERS env",
"Brokers": "localhost:9092",
Copy link
Contributor

Choose a reason for hiding this comment

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

change isn't needed

kafka/kafka.go Outdated Show resolved Hide resolved
@@ -1,7 +1,7 @@
package kafka

// Copyright 2016-2023 Confluent Inc.
// AUTOMATICALLY GENERATED ON 2023-07-12 11:45:05.970954589 +0200 CEST m=+0.000441527 USING librdkafka 2.2.0
// AUTOMATICALLY GENERATED ON 2023-09-29 04:54:21.039467052 +0530 IST m=+0.006323752 USING librdkafka 2.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

don't add this file to the change, we'll do it with the release

kafka/kafka.go Outdated Show resolved Hide resolved
kafka/kafka.go Outdated Show resolved Hide resolved
@pranavrth pranavrth force-pushed the dev_add-topic-id-to-describe-group-response branch from e7de1b1 to 34f496c Compare October 11, 2023 04:50
@pranavrth pranavrth force-pushed the dev_add-topic-id-to-describe-group-response branch from 452f402 to d7dc5af Compare October 12, 2023 19:00
@pranavrth pranavrth marked this pull request as ready for review October 12, 2023 19:02
kafka/integration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

Approved with one minor comment - also need to merge the latest changes to the kip-430 branch.

Base automatically changed from feature/kip430 to master October 19, 2023 06:06
@@ -1420,6 +1420,7 @@ func (its *IntegrationTestSuite) TestAdminClient_DescribeTopics() {
"Expected correct error for nonexistent topic")

topicDesc := topicDescs[0]
assert.NotNil(topicDesc.TopicID)
Copy link
Contributor

Choose a reason for hiding this comment

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

assert.NotNil asways returns true for structs according to this issue stretchr/testify#570
Suggest:

Suggested change
assert.NotNil(topicDesc.TopicID)
assert.NotZero(topicDesc.TopicID.GetLeastSignificantBits())
assert.NotZero(topicDesc.TopicID.GetMostSignificantBits())
assert.NotEmpty(topicDesc.TopicID.String())

@pranavrth pranavrth merged commit b4a69cc into master Oct 21, 2023
1 check passed
@pranavrth pranavrth deleted the dev_add-topic-id-to-describe-group-response branch October 21, 2023 14:49
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.

4 participants