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

*: Pare down usage of buf linting #30339

Closed
wants to merge 2 commits into from

Conversation

ParkMyCar
Copy link
Member

@ParkMyCar ParkMyCar commented Nov 4, 2024

This PR pares down our usage of the buf protobuf linting tool by marking most of the files as ignored. There are relatively few files we actually need to maintain backward compatibility for, most are used for communication between environmentd and clusterd which are guaranteed to be running the same version of Materialize.

Concretely, this changes bin/lint to require protobuf files to be marked with buf breaking: (ignore | check) and updates all of the files not currently marked.

Motivation

buf linting has tripped us up before by requiring backwards compatibility where it's not needed. See this Slack thread.

Tips for reviewer

What really needs review here are that the protobuf files are correctly marked ignore or check

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ParkMyCar ParkMyCar requested review from aljoscha, danhhz, jkosh44, a team and teskje as code owners November 4, 2024 18:46
Copy link
Contributor

Choose a reason for hiding this comment

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

For the repr modules, how did you determine which one were written down and which weren't?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked where the messages were used, they were generally used in the errors returned from functions (not durably persisted) or in the ProtoRow encoding (durably persisted). More than happy to just mark everything in repr as check if you think that's better

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, marking the whole crate as check feels much safer to me unless there's an obvious and quick way to validate that some file is or isn't persisted.

@antiguru
Copy link
Member

antiguru commented Nov 5, 2024

Not strongly opposed, but past experience tells me this eventually will break someone's code. I've seen incidents in the past where something silently started writing down a proto that wasn't considered to have stability requirements, but through a long dependency chain still was serialized. Ideally, we have a lint that checks all protos ever recorded durably only depend on protos with backwards compatibility, but I'm not sure that exists.

Otoh, this doesn't move us to a much worse place than we're currently in, and we'll need to revisit all of our proto infrastructure if we start storing protos durably outside persist.

@ParkMyCar
Copy link
Member Author

not necessary any more

@ParkMyCar ParkMyCar closed this Jan 13, 2025
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.

3 participants