Skip to content
This repository has been archived by the owner on Jun 5, 2024. It is now read-only.

feat: implement consumer-group service (not exposed) #560

Merged
merged 1 commit into from
Dec 16, 2022

Conversation

GGabriele
Copy link
Contributor

No description provided.

@GGabriele GGabriele requested a review from a team as a code owner December 16, 2022 08:37
@GGabriele GGabriele force-pushed the feat/consumer-group-service branch 2 times, most recently from 79dfa8f to c4b2ce2 Compare December 16, 2022 08:57
Copy link
Contributor

@javierguerragiraldez javierguerragiraldez left a comment

Choose a reason for hiding this comment

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

LGTM. the couple of nits i mentioned shouldn't affect execution at all

return nil, s.err(ctx, err)
}

db, err := s.CommonOpts.getDB(ctx, req.Cluster)
Copy link
Contributor

Choose a reason for hiding this comment

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

very tiny nit: CommonOpts isn't needed here. s.getDB(...) is just as valid (and avoid gopls complains about this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, thanks for pointing it out!

cg := &v1.ConsumerGroup{
Name: "foo",
}
res := c.GET("/v1/consumer-groups").WithJSON(cg).Expect()
Copy link
Contributor

Choose a reason for hiding this comment

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

i know this is a placeholder, but GET typically don't have a body:

Suggested change
res := c.GET("/v1/consumer-groups").WithJSON(cg).Expect()
res := c.GET("/v1/consumer-groups").Expect()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

totally!

@GGabriele GGabriele merged commit 62837fc into main Dec 16, 2022
@GGabriele GGabriele deleted the feat/consumer-group-service branch December 16, 2022 12:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants