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

Support describeacls #1166

Merged
merged 19 commits into from
Jul 28, 2023
Merged

Support describeacls #1166

merged 19 commits into from
Jul 28, 2023

Conversation

petedannemann
Copy link
Contributor

@petedannemann petedannemann commented Jul 18, 2023

Support describeacls API

API Docs

I based this PR on similar work done here #1119

Additionally I fixed the tags for createacls protocol as I noticed those did not match the API docs

@petedannemann petedannemann marked this pull request as draft July 18, 2023 14:16
@petedannemann petedannemann marked this pull request as ready for review July 18, 2023 17:56
@rhansen2 rhansen2 self-assigned this Jul 21, 2023
createacl_test.go Outdated Show resolved Hide resolved
createacl_test.go Outdated Show resolved Hide resolved
createacl_test.go Show resolved Hide resolved
@petedannemann petedannemann marked this pull request as draft July 21, 2023 17:37
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this file to match the file name convention of the corresponding source code file https://github.com/segmentio/kafka-go/blob/main/createacls.go

@petedannemann petedannemann marked this pull request as ready for review July 24, 2023 18:37
@@ -15,15 +15,18 @@ func TestClientCreateACLs(t *testing.T) {
client, shutdown := newLocalClient()
defer shutdown()

res, err := client.CreateACLs(context.Background(), &CreateACLsRequest{
topic := makeTopic()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made these non-deterministic to avoid collisions with other tests

type ACLFilter struct {
ResourceTypeFilter ResourceType
ResourceNameFilter string
ResourcePatternTypeFilter PatternType
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure how to handle this field since it was added in v1. We could use a pointer to make this nullable or have separate data structures and code paths for different versions. WDYT @rhansen2 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this works as is but we should probably have a comment explaining that it won't be used if the only supported API version is v0. I think the encoding will leave it out as appropriate based on the api version being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I added a comment

@petedannemann petedannemann merged commit f4ca0b4 into segmentio:main Jul 28, 2023
@petedannemann petedannemann deleted the describeacls branch July 28, 2023 16:54
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants