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

Add list acls endpoint for the new programmatic API #896

Merged
merged 8 commits into from
Oct 24, 2023
Merged

Conversation

weeco
Copy link
Contributor

@weeco weeco commented Oct 20, 2023

Draft PR for adding the ListACLs endpoint.

An integration test is missing before this PR can be un-drafted.

@weeco weeco marked this pull request as ready for review October 24, 2023 11:48
@weeco weeco requested a review from bojand October 24, 2023 11:48
v1alpha1.ACL_RESOURCE_TYPE_GROUP: fmt.Sprintf("%vgroup", resourceNamePrefix),
v1alpha1.ACL_RESOURCE_TYPE_TOPIC: fmt.Sprintf("%vtopic", resourceNamePrefix),
v1alpha1.ACL_RESOURCE_TYPE_TRANSACTIONAL_ID: fmt.Sprintf("%vtransactional-id", resourceNamePrefix),
v1alpha1.ACL_RESOURCE_TYPE_CLUSTER: "kafka-cluster",
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit or check, but this key doesn't seem to be used? Should it be used somewhere? or removed?

Copy link
Member

Choose a reason for hiding this comment

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

Nvm we iterate over the map.

repeated Policy acls = 4;
}

repeated Resource resources = 1;
}

message CreateACLRequest {
Copy link
Member

Choose a reason for hiding this comment

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

Should we add some buf validation annotations to the requests?

Copy link
Contributor Author

@weeco weeco Oct 24, 2023

Choose a reason for hiding this comment

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

The CreateACL and DeleteACL Request is still to be implemented. It's only there because of the requested OpenAPI spec

@weeco weeco merged commit 540e8fe into master Oct 24, 2023
6 checks passed
@weeco weeco deleted the add-list-acls branch October 24, 2023 14:48
@bojand bojand restored the add-list-acls branch April 12, 2024 00:25
@bojand bojand deleted the add-list-acls branch April 12, 2024 00:38
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.

2 participants