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

feat: add new kafka topic configuration properties #481

Merged

Conversation

gokerakc
Copy link
Contributor

@gokerakc gokerakc commented Jan 8, 2024

Description
The following updates are introduced for the kafka topic configuration object:

  • Updated additionalProperties as true.
  • Added 4 new properties as proposed in the issue.

Please check the related issue for more details.

Note: Once we are happy with the changes I'll create another PR to update the bindings' README file in the other repo.

Related issue(s)
Resolves the following issue: asyncapi/bindings#231

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@dpwdec
Copy link
Collaborator

dpwdec commented Jan 10, 2024

Very much needed! 👍 We're struggling to work around these gaps with x- extensions.

@derberg
Copy link
Member

derberg commented Jan 25, 2024

Hey, please just remember that JSON Schema is just a tool, not a spec itself so whatever is added to JSON Schema, first need to be added to Kafka Binding spec -> https://github.com/asyncapi/bindings/tree/master/kafka

@gokerakc gokerakc marked this pull request as draft January 29, 2024 14:20
@gokerakc
Copy link
Contributor Author

I've updated this PR as a draft because the following PR needs to be reviewed first:

@gokerakc gokerakc force-pushed the update_kafkaTopicConfigurationObject branch 2 times, most recently from b643e30 to abe48ab Compare February 12, 2024 11:59
@gokerakc gokerakc marked this pull request as ready for review February 12, 2024 12:02
@gokerakc
Copy link
Contributor Author

Given the related bindings PR (asyncapi/bindings#238) got merged I think we can continue with this one.

I've added a new schema version (0.5.0) to reflect the new changes in the kafka topic configuration object.

It would be great if you could re-review this PR @dalelane.

@gokerakc gokerakc requested a review from dalelane February 12, 2024 12:08
dalelane
dalelane previously approved these changes Feb 20, 2024
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

sorry for the delay in replying - I was on vacation last week

this looks good to me - thanks for the updates

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@gokerakc gokerakc marked this pull request as draft February 20, 2024 16:47
@gokerakc gokerakc force-pushed the update_kafkaTopicConfigurationObject branch from ef3aa1f to f1403e0 Compare February 20, 2024 16:53
@gokerakc gokerakc marked this pull request as ready for review February 20, 2024 16:58
@gokerakc
Copy link
Contributor Author

Sorry guys, I've messed around with my commits. I had a problem verifying my signature but now it's fixed.

It seems like extra checks (Test NodeJS PR ones) were added because of my commit changes. If it's necessary, I can close this PR and create a new one.

Can you please re-approve the PR? @dalelane

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@gokerakc
Copy link
Contributor Author

/rtm

@gokerakc
Copy link
Contributor Author

Hey guys, can you please approve that 1 workflow? It seems like we need to run that workflow before the merge. @dalelane @smoya

@derberg derberg changed the title feat: added new kafka topic configuration properties feat: add new kafka topic configuration properties Feb 21, 2024
@gokerakc
Copy link
Contributor Author

/rtm

@asyncapi-bot asyncapi-bot merged commit ae96600 into asyncapi:master Feb 21, 2024
15 checks passed
@gokerakc gokerakc deleted the update_kafkaTopicConfigurationObject branch February 21, 2024 15:47
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@gokerakc hey, I see we overlooked and did not make sure you add new version to proper definitions. Can you add 0.5.0 to:

and I do not only mean an enum, to add it to enum, but also respective if statement?

if you do it just ping me and I will review and merge with higher priority

as for now, new version is not really in place as anyway documents with 0.5.0 binding are not accepted as valid

dalelane added a commit to dalelane/spec-json-schemas that referenced this pull request Mar 14, 2024
cf. spotted in a comment in
asyncapi#481

Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
@dalelane
Copy link
Collaborator

@derberg @gokerakc sorry I missed that when I did the review - I've added a PR now at #501

dalelane added a commit to dalelane/spec-json-schemas that referenced this pull request Mar 14, 2024
cf. spotted in a comment in
asyncapi#481

Signed-off-by: Dale Lane <dale.lane@uk.ibm.com>
@gokerakc
Copy link
Contributor Author

Thanks @dalelane!

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.

5 participants