Closed ilkerkocatepe closed 1 day ago
Hi @ppatierno, thank you for the review, I updated the files which you mentioned.
@ilkerkocatepe thanks for addressing my comments but thinking more on your API proposal I have a more "destructive" one :-)
This proposal is about using the /admin/topics/{topicName}
URL where you specify the topic name as part of the path and then passing replication factor and partitions via query parameters. While it would work, I think it's not that much extensible for future additions like allowing to specify topic configuration when creating a topic.
I think we should go in a different direction by using a JSON as body of the HTTP POST request where you can specify parameters. What I have in mind is something like this, let me know what you think ...
The URL would be just /admin/topics/
and the POST request could have a body like the following:
{
"topic_name": "my-new-topic",
"partitions_count": 5,
"replication_factor": 3,
}
The above JSON would be extensible in the future, allowing to have a "configs" field for example with a properties map for topic configuration parameters. Of course, it means changing the OpenAPI spec as well and providing examples of body within the spec as well. I am really sorry to realize this later and that we were going in a wrong direction. Wdyt?
You are absolutely right. Also, it will be proper way according to REST standards. I would like to edit implementation :)
@ilkerkocatepe great to hear that you are still onboard! My apologies for realising this so late. Thanks for the effort!
@ppatierno no worries, I develop similar things at work too, I should think about it :)
Hi again @ppatierno, could you review again :)
Hi @ppatierno, I updated points that you mentioned. But I have a question, I added required tag for topic_name and now if request body doesn't contain topic_name, endpoint gives 400 code. So I changed test response to BAD_REQUEST. What do you think about validations on HttpAdminBridgeEndpoint.doCreateTopic?
Hi @ppatierno thank you for review, and sorry for delay. the pr is ready for review again :)
@ilkerkocatepe other than my last comments, we would need a line in the CHANGELOG about this addition. Thanks!
Hi again @ppatierno, I committed issues that you mentioned, but something happened weird when rebasing, I think due to use of IDE version controlling.
@ilkerkocatepe as Jakub said you have something which belongs to an already existing commit from here https://github.com/strimzi/strimzi-kafka-bridge/commit/ea03491fa72e3471345a1e7cf8eedbcacbce794c Can you fix the PR please?
@ilkerkocatepe we are planning to start releasing bridge 0.31.0 RC1 by the end of the week and it would be great to have this in. Do you think you are able clean this PR (removing the unrelated commit) without opening a new one and copying over the code there? Otherwise I will just merge this .
I think I messed up everything and need help :/
@ilkerkocatepe I just merged it. Thanks!
This PR resolves issue https://github.com/strimzi/strimzi-kafka-bridge/issues/434