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

Sync one more gofeatures reference #822

Merged
merged 6 commits into from
Jan 9, 2025
Merged

Conversation

unmultimedio
Copy link
Member

@unmultimedio unmultimedio commented Jan 7, 2025

Backend BSR currently does not respect buf.yaml dependency pins and instead it uses the latest synced dependency reference, as stated in the README, which means:

  • All gofeatures synced references always depended at the latest wellknowntypes available at the moment, not specifically v27.0 (as its synced buf.yaml suggested). Right now it depends on v29.1.
  • Latest gofeatures reference v1.36.1 is currently stuck and unsyncable in BSRs because it does not build, due to a duplicate proto path: go_features.proto file, recently added to wellknowntypes module in v29.2.

Fix plan is:

  • Make sure this is the only buf.yaml that pins to a specific dependency.
  • Mark [email protected] reference as skippable, so it skips to v1.36.2 from this PR.
  • Set the BSR to respect the pinned deps (as long as they're synced references/labels). We don't want to do this step before, because the gofeatures dependency to wellknowntypes would jump backwards from v29.1 (what currently depends on), to v27.0 (what's synced in the central bucket).

Then we can merge this PR and release modules, so BSR picks up this new reference and syncs it.

@unmultimedio unmultimedio changed the title Sync one more gofeatures reference Sync one more gofeatures reference Jan 7, 2025
Copy link

github-actions bot commented Jan 7, 2025

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedJan 9, 2025, 3:38 PM

@jhump
Copy link
Member

jhump commented Jan 7, 2025

Would it be better to instead remove the buf.yaml from this module? Since they are well-known, buf and the BSR can compile them without an explicit dependency. The reason we added the dependency was to make sure this module would compile even when using old versions of the buf CLI (which might embed the well-known imports for a version prior to v27.0, which would fail to compile).

I don't understand pinning it to the newer 29.1 of wellknowntypes. That will never be able to compile, due to the duplicate google/protobuf/go_features.proto file, right?

@pkwarren
Copy link
Member

pkwarren commented Jan 7, 2025

I don't understand pinning it to the newer 29.1 of wellknowntypes. That will never be able to compile, due to the duplicate google/protobuf/go_features.proto file, right?

v29.1 is the last release of WKT that didn't have go_features.proto, so it should be ok to list here. See https://buf.build/protocolbuffers/wellknowntypes/tree/v29.1:google/protobuf.

@unmultimedio
Copy link
Member Author

v29.1 is the last release of WKT that didn't have go_features.proto, so it should be ok to list here. See https://buf.build/protocolbuffers/wellknowntypes/tree/v29.1:google/protobuf.

Correct, to be more clear: v29.1 is the last release of WKT in the BSR that didn't have go_features.proto.

If you look at Github, WKT actually included go_features.proto in v29.0, but we missed it and actually added it to the modules sync in v29.2.

@jhump
Copy link
Member

jhump commented Jan 7, 2025

Ack about 29.1 not actually including the file in the BSR's version of protocolbuffers/wellknowntypes.

Will it be a lot of work to make the BSR respect the version pins in these modules' buf.yaml? If we are deprecating the module anyway, maybe it's okay to just drop its buf.yaml?

In any event, this PR looks good to me. I'm just asking if there's some faster (but still sane/acceptable) path to getting the deprecated module sync'ed to BSRs.

@unmultimedio
Copy link
Member Author

Will it be a lot of work to make the BSR respect the version pins in these modules' buf.yaml? If we are deprecating the module anyway, maybe it's okay to just drop its buf.yaml?

I don't expect it to be a lot, currently working on it now. We originally decided not to support it to avoid a foot gun and set commit IDs there (which would be different across BSR instances), but as long as we use synced references (ie labels), we should be good.

@bufbuild bufbuild deleted a comment from saquibmian Jan 7, 2025
@unmultimedio unmultimedio marked this pull request as ready for review January 9, 2025 15:53
@unmultimedio unmultimedio merged commit 3f5cbd2 into main Jan 9, 2025
6 checks passed
@unmultimedio unmultimedio deleted the jfigueroa/unblock-gofeatures branch January 9, 2025 16:06
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