-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: change /metadata REST path to /v1/metadata #3467
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one question:
The path is also included, which for now is empty.
What will it be in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @spena
I'm left wondering what the path
and id
are for. Both are empty strings. Knowing little about this change, I would of thought that the id
field of the meta request would return the id of either the Ksql cluster or this specific node.
ksql-rest-model/src/test/java/io/confluent/ksql/rest/entity/ServerClusterIdTest.java
Show resolved
Hide resolved
@big-andy-coates @vcrfxia I updated the PR to fix the assertion and add a couple of comments of what ID and PATH will be used in the future. ID will be used to create a single URL string that includes the kafka & ksql cluster names which will be used by authorization services (as a CRN authority) to authorize access to this cluster. There's a plan to include this in 5.5 or later. PATH is unsure what will be, but it seems it will contain the Cloud account organization where this cluster belongs to. |
Description
To be consistent with other Confluent platforms, the
/metadata
and/metadata/id
paths were renamed to/v1/metadata
and/v1/metadata/id
.Also, the
/v1/metadata/id
modified the output to this:The kafka/ksql clusters ID are wrapped inside a
clusters
section. Thepath
is also included, which for now is empty.Testing done
Reviewer checklist