Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- Fix `buf convert` to allow for zero length for `binpb`, `txtpb`, and `yaml` formats.
- Fix use of deprecated flag `--include-types` for `buf generate`.
- Add `--against-registry` flag to `buf breaking` that runs breaking checks against the latest
commit on the default branch of the corresponding module in the registry.

## [v1.50.1] - 2025-03-10

Expand Down
9 changes: 8 additions & 1 deletion private/buf/bufctl/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,13 @@ import (
"google.golang.org/protobuf/proto"
)

// ImageWithConfig pairs an Image with lint and breaking configuration.
// ImageWithConfig pairs an Image with its corresponding [bufmodule.Module] full name
// (which may be nil), [bufmodule.Module] opaque ID, and lint and breaking configurations.
type ImageWithConfig interface {
bufimage.Image

ModuleFullName() bufparse.FullName
ModuleOpaqueID() string
Comment on lines +62 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this could be Module() bufmodule.Module and document that it may be nil? From the docs it seems like the Module.FullName might be empty (like in Module) not that it isn't a Module, as the opaque ID can be empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

The OpaqueID should always be present:

// OpaqueID returns an unstructured ID that can uniquely identify a Module relative
// to other Modules it was built with from a ModuleSetBuilder.
//
// Always present, regardless of whether a Module was provided by a ModuleProvider,
// or built with a ModuleSetBuilder.
//
// An OpaqueID can be used to denote expected uniqueness of content; if two Modules
// have different IDs, they should be expected to be logically different Modules.
//
// An OpaqueID can be used as a human-readable identifier of the Module, suitable for printing
// to a console. However, the OpaqueID may contain information on local directory structure, so
// do not log or print it in contexts where such information may be sensitive.
//
// An OpaqueID's structure should not be relied upon, and is not a globally-unique identifier.
// It's uniqueness property only applies to the lifetime of the Module, and only within
// Modules commonly built from a ModuleSetBuilder.
//
// If two Modules have the same FullName, they will have the same OpaqueID.
OpaqueID() string

My reasoning for not carrying the entire Module around is because the Image should be a compiled version of the Module (and the Module is otherwise lazily loaded) -- so it seems unnecessary... but maybe it's easier?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed outside of the PR: this is mostly for the case of messageRef, where we resolve the message through the bucket without the Module, so there is no FullName or OpaqueID. In those cases, we are not able to use --against-registry flag.

LintConfig() bufconfig.LintConfig
BreakingConfig() bufconfig.BreakingConfig
PluginConfigs() []bufconfig.PluginConfig
Expand Down Expand Up @@ -476,6 +479,8 @@ func (c *controller) GetTargetImageWithConfigsAndCheckClient(
imageWithConfigs := []ImageWithConfig{
newImageWithConfig(
image,
nil, // No module name for a single message ref
"", // No module opaque ID for a single message ref
lintConfig,
breakingConfig,
pluginConfigs,
Expand Down Expand Up @@ -1170,6 +1175,8 @@ func (c *controller) buildTargetImageWithConfigs(
imageWithConfigs,
newImageWithConfig(
image,
module.FullName(),
module.OpaqueID(),
workspace.GetLintConfigForOpaqueID(module.OpaqueID()),
workspace.GetBreakingConfigForOpaqueID(module.OpaqueID()),
workspace.PluginConfigs(),
Expand Down
15 changes: 15 additions & 0 deletions private/buf/bufctl/image_with_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,45 @@ package bufctl
import (
"github.com/bufbuild/buf/private/bufpkg/bufconfig"
"github.com/bufbuild/buf/private/bufpkg/bufimage"
"github.com/bufbuild/buf/private/bufpkg/bufparse"
)

type imageWithConfig struct {
bufimage.Image

moduleFullName bufparse.FullName
moduleOpaqueID string
lintConfig bufconfig.LintConfig
breakingConfig bufconfig.BreakingConfig
pluginConfigs []bufconfig.PluginConfig
}

func newImageWithConfig(
image bufimage.Image,
moduleFullName bufparse.FullName,
moduleOpaqueID string,
lintConfig bufconfig.LintConfig,
breakingConfig bufconfig.BreakingConfig,
pluginConfigs []bufconfig.PluginConfig,
) *imageWithConfig {
return &imageWithConfig{
Image: image,
moduleFullName: moduleFullName,
moduleOpaqueID: moduleOpaqueID,
lintConfig: lintConfig,
breakingConfig: breakingConfig,
pluginConfigs: pluginConfigs,
}
}

func (i *imageWithConfig) ModuleFullName() bufparse.FullName {
return i.moduleFullName
}

func (i *imageWithConfig) ModuleOpaqueID() string {
return i.moduleOpaqueID
}

func (i *imageWithConfig) LintConfig() bufconfig.LintConfig {
return i.lintConfig
}
Expand Down
24 changes: 24 additions & 0 deletions private/buf/cmd/buf/buf_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3558,6 +3558,30 @@ testdata/check_plugins/current/proto/common/v1/breaking.proto:18:1:Enum "common.
)
}

func TestBreakingAgainstRegistryFlag(t *testing.T) {
t.Parallel()
testRunStderr(
t,
nil,
1,
"Failure: Cannot set both --against and --against-registry",
"breaking",
filepath.Join("testdata", "check_plugins", "current", "proto"),
"--against",
filepath.Join("testdata", "check_plugins", "previous", "proto"),
"--against-registry",
)
testRunStderr(
t,
nil,
1,
"Failure: cannot use --against-registry with unnamed module, testdata/success",
"breaking",
filepath.Join("testdata", "success"),
"--against-registry",
)
}

func TestVersion(t *testing.T) {
t.Parallel()
testRunStdout(t, nil, 0, bufcli.Version, "--version")
Expand Down
101 changes: 85 additions & 16 deletions private/buf/cmd/buf/command/breaking/breaking.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/bufbuild/buf/private/pkg/app/appext"
"github.com/bufbuild/buf/private/pkg/slicesext"
"github.com/bufbuild/buf/private/pkg/stringutil"
"github.com/bufbuild/buf/private/pkg/syserror"
"github.com/bufbuild/buf/private/pkg/wasm"
"github.com/spf13/pflag"
)
Expand All @@ -42,6 +43,7 @@ const (
configFlagName = "config"
againstFlagName = "against"
againstConfigFlagName = "against-config"
againstRegistryFlagName = "against-registry"
excludePathsFlagName = "exclude-path"
disableSymlinksFlagName = "disable-symlinks"
)
Expand Down Expand Up @@ -77,6 +79,7 @@ type flags struct {
Config string
Against string
AgainstConfig string
AgainstRegistry bool
ExcludePaths []string
DisableSymlinks bool
// special
Expand Down Expand Up @@ -129,7 +132,8 @@ Overrides --%s`,
againstFlagName,
"",
fmt.Sprintf(
`Required. The source, module, or image to check against. Must be one of format %s`,
`Required, except if --%s is set. The source, module, or image to check against. Must be one of format %s`,
againstRegistryFlagName,
buffetch.AllFormatsString,
),
)
Expand All @@ -139,14 +143,24 @@ Overrides --%s`,
"",
`The buf.yaml file or data to use to configure the against source, module, or image`,
)
flagSet.BoolVar(
&f.AgainstRegistry,
againstRegistryFlagName,
false,
fmt.Sprintf(
`Run breaking checks against the latest commit on the default branch in the registry. All modules in the input must have a name configured, otherwise this will fail.
If a remote module is not found with the configured name, then this will fail. This cannot be set with --%s.`,
againstFlagName,
),
)
}

func run(
ctx context.Context,
container appext.Container,
flags *flags,
) (retErr error) {
if err := bufcli.ValidateRequiredFlag(againstFlagName, flags.Against); err != nil {
if err := validateFlags(flags); err != nil {
return err
}
input, err := bufcli.GetInputValue(container, flags.InputHashtag, ".")
Expand Down Expand Up @@ -194,19 +208,64 @@ func run(
return err
}
}
// Do not exclude imports here. bufcheck's Client requires all imports.
// Use bufcheck's BreakingWithExcludeImports.
againstImageWithConfigs, _, err := controller.GetTargetImageWithConfigsAndCheckClient(
ctx,
flags.Against,
wasm.UnimplementedRuntime,
bufctl.WithTargetPaths(externalPaths, flags.ExcludePaths),
bufctl.WithConfigOverride(flags.AgainstConfig),
)
if err != nil {
return err
var againstImages []bufimage.Image
if flags.Against != "" {
// Do not exclude imports here. bufcheck's Client requires all imports.
// Use bufcheck's BreakingWithExcludeImports.
againstImagesWithConfigs, _, err := controller.GetTargetImageWithConfigsAndCheckClient(
ctx,
flags.Against,
wasm.UnimplementedRuntime,
bufctl.WithTargetPaths(externalPaths, flags.ExcludePaths),
bufctl.WithConfigOverride(flags.AgainstConfig),
)
if err != nil {
return err
}
// We do not require the check configs from the against target once built, so they can
// be dropped here.
againstImages, err = slicesext.MapError(
againstImagesWithConfigs,
func(imageWithConfig bufctl.ImageWithConfig) (bufimage.Image, error) {
againstImage, ok := imageWithConfig.(bufimage.Image)
if !ok {
return nil, syserror.New("imageWithConfig could not be converted to Image")
}
return againstImage, nil
},
)
if err != nil {
return err
}
}
if len(imageWithConfigs) != len(againstImageWithConfigs) {
if flags.AgainstRegistry {
for _, imageWithConfig := range imageWithConfigs {
if imageWithConfig.ModuleFullName() == nil {
if imageWithConfig.ModuleOpaqueID() == "" {
// This can occur in the case of a [buffetch.MessageRef], where we resolve the message
// ref directly from the bucket without building the [bufmodule.Module]. In that case,
// we are unnable to use --against-registry.
return fmt.Errorf("cannot use --%s with unnamed module", againstRegistryFlagName)
}
return fmt.Errorf(
"cannot use --%s with unnamed module, %s",
againstRegistryFlagName,
imageWithConfig.ModuleOpaqueID(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to check for empty opaque ID here?

Copy link
Member Author

@doriable doriable Mar 24, 2025

Choose a reason for hiding this comment

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

Re: the comment above, the OpaqueID should always be present, but I could check for an empty and return a different error under those circumstances (and a syserror).

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted this error for the buffetch.MessageRef case -- given that we don't build the module for those cases, --against-registry basically cannot be used.

)
}
againstImage, err := controller.GetImage(
ctx,
imageWithConfig.ModuleFullName().String(),
bufctl.WithTargetPaths(externalPaths, flags.ExcludePaths),
bufctl.WithConfigOverride(flags.AgainstConfig),
)
if err != nil {
return err
}
againstImages = append(againstImages, againstImage)
}
}
if len(imageWithConfigs) != len(againstImages) {
// If workspaces are being used as input, the number
// of images MUST match. Otherwise the results will
// be meaningless and yield false positives.
Expand All @@ -216,7 +275,7 @@ func run(
return fmt.Errorf(
"input contained %d images, whereas against contained %d images",
len(imageWithConfigs),
len(againstImageWithConfigs),
len(againstImages),
)
}
// We add all check configs (both lint and breaking) as related configs to check if plugins
Expand All @@ -240,7 +299,7 @@ func run(
ctx,
imageWithConfig.BreakingConfig(),
imageWithConfig,
againstImageWithConfigs[i],
againstImages[i],
breakingOptions...,
); err != nil {
var fileAnnotationSet bufanalysis.FileAnnotationSet
Expand Down Expand Up @@ -274,3 +333,13 @@ func getExternalPathsForImages[I bufimage.Image, S ~[]I](images S) ([]string, er
}
return slicesext.MapKeysToSlice(externalPaths), nil
}

func validateFlags(flags *flags) error {
if flags.Against == "" && !flags.AgainstRegistry {
return fmt.Errorf("Must set --%s or --%s", againstFlagName, againstRegistryFlagName)
}
if flags.Against != "" && flags.AgainstRegistry {
return fmt.Errorf("Cannot set both --%s and --%s", againstFlagName, againstRegistryFlagName)
}
return nil
}