buf icon indicating copy to clipboard operation
buf copied to clipboard

Add lint rule to prevent stable packages importing unstable packages

Open noahseger opened this issue 1 year ago • 5 comments

A common mistake we see is forgetting a v1alpha import into a v1 package. Has a lint rule preventing this been considered? If not, what do folks here generally use to lint better on top of Buf?

I think the general community would benefit from baking this into the DEFAULT configuration though, because even changing a field from v1alpha.MyType to v1.MyType is breaking, and requires the ignore / commit / unignore / commit dance.

noahseger avatar May 16 '24 21:05 noahseger

I'm not sure I understand the question or proposal, can you explain a bit more what you are looking for?

bufdev avatar May 18 '24 22:05 bufdev

@bufdev say I have two protobufs like:

syntax = "proto3";

package examples.v1alpha;

message AlphaDependency {
  string foo = 1;
}
syntax = "proto3";

import "examples/v1alpha/dependency.proto"

package examples.v1;

message StableExample {
  examples.v1alpha.AlphaDependency dependency = 1;
}

There are two primary problems:

  1. Buf (by default) allows me to change the v1alpha dependency in incompatible ways, which silently breaks v1
  2. If I want to promote the v1alpha dependency to v1, the type change in the existing stable file is breaking

noahseger avatar May 21 '24 20:05 noahseger

  • By default, buf does not allow you to make breaking changes to the v1alpha package, that is an option you have to opt into via https://buf.build/docs/configuration/v2/buf-yaml#ignore_unstable_packages. If ignore_unstable_packages is not set, then breaking changes to v1alpha will result in errors when running buf breaking.
  • Changing the type of a field is considered a breaking change, so updating the type of examples.v1alpha.AlphaDependency to say examples.v1.AlphaDependency will result in an error when running buf breaking. There's no concrete way for buf breaking to denote that "this was just a package change, the underlying type is exactly the same, do not error", and we wouldn't want to ignore this as a general concern. We could, in theory, do deep type inspection, but this would be source-code-breaking to begin with, and we consider that out of scope. Your mitigation here is to override the breaking change error if you are certain that your change is expected.

bufdev avatar May 21 '24 20:05 bufdev

Ah yes, understood — we did enable ignore_unstable_packages because we need to evolve alpha versions in breaking ways.

But to be clear, the feature request isn't to inspect the alpha dependency types (although I think this would work with PACKAGE as the breaking configuration type?).

The feature request is:

  • when I have a stable proto file
  • and I import an unstable proto file into it
  • then I get a lint error like "Stable protos may not import unstable protos"

noahseger avatar May 22 '24 17:05 noahseger

OK I understand the feature request now - thanks for the clarification. I've opened #3010 to discuss this issue further.

bufdev avatar May 23 '24 15:05 bufdev

This has been added and will go out in a release tomorrow morning.

bufdev avatar Aug 22 '24 04:08 bufdev

Thank you all so much! Upgrading now.

noahseger avatar Sep 04 '24 19:09 noahseger