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

Add --against-registry flag for buf breaking #3696

Merged
merged 7 commits into from
Mar 24, 2025

Conversation

doriable
Copy link
Member

@doriable doriable commented Mar 18, 2025

This PR adds a --against-registry flag to buf breaking that can be
set in lieu of --against. When --against-registry is set, breaking checks
are run against the latest commit on the default label of the remote module
in the registry.

buf breaking will error if one of --against or --against-registry is
not set:

$ buf breaking
Failure: Must set --against or --against-registry

buf breaking --against-registry will error if not all modules in the input
are named (and thus a remote cannot be resolved):

$ buf breaking --against-registry
Failure: cannot use --against-registry with unnamed module, proto/local

Fixes #3654

Copy link
Contributor

github-actions bot commented Mar 18, 2025

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMar 24, 2025, 7:07 PM

@doriable doriable requested review from emcfarlane and bufdev March 18, 2025 17:15
Comment on lines +62 to +63
ModuleFullName() bufparse.FullName
ModuleOpaqueID() string
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.

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.

@doriable doriable requested a review from emcfarlane March 24, 2025 19:50
@doriable doriable merged commit 1ce3aac into main Mar 24, 2025
10 checks passed
@doriable doriable deleted the 3654-breaking-against-registry branch March 24, 2025 21:05
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.

Improvements to running buf breaking for workspaces
2 participants