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

[#434] feat: added create topic endpoint #929

Merged
merged 58 commits into from
Nov 21, 2024

Conversation

ilkerkocatepe
Copy link
Contributor

This PR resolves issue #434

@ppatierno ppatierno added this to the 0.31.0 milestone Oct 7, 2024
src/main/resources/openapi.json Outdated Show resolved Hide resolved
src/main/resources/openapi.json Outdated Show resolved Hide resolved
src/main/resources/openapi.json Outdated Show resolved Hide resolved
src/main/resources/openapiv2.json Outdated Show resolved Hide resolved
@ilkerkocatepe
Copy link
Contributor Author

Hi @ppatierno,
thank you for the review, I updated the files which you mentioned.

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

@ilkerkocatepe thanks for addressing my comments. I left two more and also I think you should rebase again because you are bringing some changes in the pom.xml which are not related to your PR but commits already merged into main.

src/main/resources/openapi.json Outdated Show resolved Hide resolved
src/main/resources/openapi.json Outdated Show resolved Hide resolved
@ppatierno
Copy link
Member

ppatierno commented Oct 24, 2024

@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?

@ilkerkocatepe
Copy link
Contributor Author

You are absolutely right. Also, it will be proper way according to REST standards. I would like to edit implementation :)

@ppatierno
Copy link
Member

@ilkerkocatepe great to hear that you are still onboard! My apologies for realising this so late. Thanks for the effort!

Signed-off-by: ilkerkocatepe <[email protected]>
@ilkerkocatepe
Copy link
Contributor Author

Hi again @ppatierno,
I committed issues that you mentioned,
but something happened weird when rebasing, I think due to use of IDE version controlling.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Is it intentional that it contains the dpeendency changes etc.?

@ppatierno
Copy link
Member

ppatierno commented Nov 20, 2024

@ilkerkocatepe as Jakub said you have something which belongs to an already existing commit from here ea03491
Can you fix the PR please?

@ppatierno
Copy link
Member

@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 .

This reverts commit ea03491

Signed-off-by: ilkerkocatepe <[email protected]>
Signed-off-by: ilkerkocatepe <[email protected]>
@ilkerkocatepe
Copy link
Contributor Author

I think I messed up everything and need help :/

@ppatierno ppatierno merged commit fa55d1b into strimzi:main Nov 21, 2024
13 checks passed
@ppatierno
Copy link
Member

@ilkerkocatepe I just merged it. Thanks!

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.

4 participants