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

Introduce protobuf formatter #933

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

keejon
Copy link
Contributor

@keejon keejon commented Aug 15, 2024

About this change - What it does

This PR adds an optional formatter for protobuf schemas.
Protobuf formatter is written in go and based on the buf formatter. It is installed as Extension and introduces go as dependency. If enabled it will run on top and independent of normalization.

Why this way

Karapace protobuf normalization is not feature complete which can lead to issues if e.g. different Kafka producers are used.
This is a short term fix for those issues.
In the future either normalization will have to be improved or moved entirely to go/protocompile/buf.

@keejon keejon marked this pull request as ready for review August 16, 2024 08:17
@keejon keejon requested review from a team as code owners August 16, 2024 08:17
@keejon keejon marked this pull request as draft August 16, 2024 08:17
@keejon keejon force-pushed the keejon/protobuf-formatter-normalization branch 10 times, most recently from 2892b47 to 353fd7c Compare August 16, 2024 15:16
Copy link

github-actions bot commented Aug 16, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  karapace
  config.py
  dependency.py 49, 53-54
  schema_models.py 85-86
  schema_reader.py
  karapace/protobuf
  one_of_element.py
  proto_parser.py
  karapace/protobuf/protopace
  __init__.py
  protopace.py 17, 51, 70-73, 81-104, 154-158, 162-171, 175-184, 188-189
Project Total  

This report was generated by python-coverage-comment-action

@keejon keejon marked this pull request as ready for review August 16, 2024 16:28
}

extend Foo {
option (my_option) = "my_value";
Copy link
Contributor Author

@keejon keejon Aug 16, 2024

Choose a reason for hiding this comment

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

Removed this because extending with options seems to be invalid proto.

Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

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

First pass, some notes here and some directly between me and author.

karapace/config.py Show resolved Hide resolved
karapace/protobuf/protopace/protopace.py Outdated Show resolved Hide resolved
karapace/protobuf/protopace/protopace.py Show resolved Hide resolved
karapace/schema_models.py Outdated Show resolved Hide resolved
requirements/requirements-dev.txt Outdated Show resolved Hide resolved
requirements/requirements.txt Outdated Show resolved Hide resolved
@keejon keejon force-pushed the keejon/protobuf-formatter-normalization branch 3 times, most recently from 70dc909 to 8833da7 Compare August 29, 2024 13:37
@keejon keejon requested a review from ivanyu September 10, 2024 09:19
README.rst Outdated Show resolved Hide resolved
container/Dockerfile Outdated Show resolved Hide resolved
@keejon keejon force-pushed the keejon/protobuf-formatter-normalization branch 2 times, most recently from 41ece6c to 6a5731d Compare September 25, 2024 10:05
Protobuf formatter is written in go and based on the buf formatter. It is installed as Extension and introduces go as dependency. If enabled it will run on top and independent of normalization.
@keejon keejon force-pushed the keejon/protobuf-formatter-normalization branch from 6a5731d to 6d954e6 Compare September 26, 2024 07:15
@jjaakola-aiven jjaakola-aiven merged commit 88bff01 into main Sep 26, 2024
10 checks passed
@jjaakola-aiven jjaakola-aiven deleted the keejon/protobuf-formatter-normalization branch September 26, 2024 08:12
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.

2 participants