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(kafka): allow protobuf schema #1369

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

Serpentiel
Copy link
Contributor

About this change—what it does

allows usage of protobuf schema in Kafka resources

resolves #1366

Why this way

we would want to have this feature in place

@Serpentiel Serpentiel added the enhancement New feature or request label Sep 26, 2023
@Serpentiel Serpentiel requested a review from a team September 26, 2023 21:54
@Serpentiel Serpentiel force-pushed the aleks-feat-kafka-allow-protobuf-schema branch from 38f3a0e to 176f225 Compare September 26, 2023 21:55
Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

LGTM, the only proposition of my would be to use a go-protobuf library for validation and normalization

@Serpentiel Serpentiel force-pushed the aleks-feat-kafka-allow-protobuf-schema branch from 176f225 to 4f49dc4 Compare September 27, 2023 16:58
@Serpentiel Serpentiel force-pushed the aleks-feat-kafka-allow-protobuf-schema branch 2 times, most recently from 002c58e to 450a5e7 Compare October 2, 2023 08:17
@Serpentiel Serpentiel force-pushed the aleks-feat-kafka-allow-protobuf-schema branch from 450a5e7 to d3980d1 Compare October 2, 2023 08:19
@Serpentiel
Copy link
Contributor Author

I don't see any reason having such a validation. It basically checks that string contains syntax = proto3; substring. Which is not a valid syntax btw.

so what do you suggest changing it with then? I checked, it works just fine as long as the string looks like syntax...=...proto3...;

we need some kind of validation in this place to be able to combine it together with the existing JSON validation mechanic

@Serpentiel Serpentiel requested a review from byashimov October 2, 2023 10:17
@Serpentiel Serpentiel force-pushed the aleks-feat-kafka-allow-protobuf-schema branch from d3980d1 to 60e27fa Compare October 2, 2023 11:04
@Serpentiel
Copy link
Contributor Author

@byashimov I've updated the PR with the fix

Copy link
Contributor

@byashimov byashimov left a comment

Choose a reason for hiding this comment

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

lgtm

@Serpentiel Serpentiel enabled auto-merge (squash) October 2, 2023 11:15
@Serpentiel Serpentiel merged commit 08deb35 into main Oct 2, 2023
10 checks passed
@Serpentiel Serpentiel deleted the aleks-feat-kafka-allow-protobuf-schema branch October 2, 2023 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for protobuf schema type for aiven_kafka_schema
3 participants