Add Describe + AlterConfigs #1014
Conversation
Mongey
force-pushed
the
cm-describe-configs
branch
from
14c65ba to
971d162
Compare
alter_configs_request.go
Outdated
| } | ||
|
|
||
| type AlterConfigsResource struct { | ||
| T ResourceType |
There was a problem hiding this comment.
I'd prefer a more useful name here... I guess Type is reserved and ResourceType is already the name of the struct, but maybe ConfigResourceType or something? Or make that the name of the struct and call the element just ResourceType?
There was a problem hiding this comment.
😊 I meant to do a second pass to rename this. I think we can use Type here, since it's type that's reserved
edit: Looks like Type will work, and I did start renaming. I'll finish off renaming🤦♂️ https://github.com/Shopify/sarama/pull/1014/files#diff-c125aeefa1915047784a716284e870f0R13
alter_configs_request.go
Outdated
|
|
||
| type ConfigEntryKV struct { | ||
| Name string | ||
| Value string |
There was a problem hiding this comment.
values are supposed to be nullable, this should probably be a *string or something?
There was a problem hiding this comment.
Whoops! you're right, I'll update
resource_type.go
Outdated
| type ResourceType int8 | ||
|
|
||
| const ( | ||
| UnknownResource ResourceType = 0 |
There was a problem hiding this comment.
curious where you got this set of constants? I don't see it in the docs anywhere?
There was a problem hiding this comment.
Took me a while to track it down again. It's from KIP-133
I can add a comment in the code
|
Thanks, this looks pretty good, just a few minor things. |
|
|
||
| type AlterConfigsResourceResponse struct { | ||
| ErrorCode int16 | ||
| ErrorMsg string |
There was a problem hiding this comment.
🤔 Maybe I should squash ErrorCode and ErrorMsg into a KError
There was a problem hiding this comment.
It’s not clear to me from the docs whether this is a KError or if it’s some other error status set? Normally KErrors don’t include a string in the actual message, just the code.
Mongey
force-pushed
the
cm-describe-configs
branch
2 times, most recently
from
be19b6a to
b2aca2b
Compare
describe_configs_request.go
Outdated
| @@ -0,0 +1,91 @@ | |||
| package sarama | |||
|
|
|||
| type Resource struct { | |||
There was a problem hiding this comment.
CI is failing because there is already a type named Resource being used for the ACL stuff
Replaces ConfigEntryKV with a map[string]*string, mimicing the API for CreateTopic
Mongey
force-pushed
the
cm-describe-configs
branch
from
b2aca2b to
9b166a1
Compare
Mongey
force-pushed
the
cm-describe-configs
branch
from
fd728c2 to
9ca6aff
Compare
|
Thanks! |
|
@Mongey It looks like you have everything for the terraform plugin for Kafka topics now. Great stuff really appreciate your work, if I can help you out in one way or another (code review or whatever), I would love too! |
No description provided.