Skip to content

Add SyncService for sync-related RPCs#2144

Merged
saquibmian merged 2 commits intomainfrom
smian/syncservice
May 31, 2023
Merged

Add SyncService for sync-related RPCs#2144
saquibmian merged 2 commits intomainfrom
smian/syncservice

Conversation

@saquibmian
Copy link
Contributor

  • Remove git-metadata RPC from Push so that Push behaviour does not diverge for sync concerns.

- Remove git-metadata RPC from Push so that Push behaviour does not diverge for sync concerns.
Comment on lines +58 to +63
// Manifest with all the module files being pushed.
buf.alpha.module.v1alpha1.Blob manifest = 4;
// Referenced blobs in the manifest. Keep in mind there is not necessarily one
// blob per file, but one blob per digest, so for files with exactly the same
// content, you can send just one blob.
repeated buf.alpha.module.v1alpha1.Blob blobs = 5;
Copy link
Member

Choose a reason for hiding this comment

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

Don't quite understand why these are present here.

  • Don't seem to be strictly part of any git metadata, it's what's pushed.
  • If manifest and blobs matches with latest, it'll attach the git metadata to that latest BSR commit,
  • If not, then it'll create a new commit (basically push effect)?

Copy link
Member

Choose a reason for hiding this comment

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

Based on our conversation, I think we want a single push RPC that also handles the git metadata. So this will be called in lieu of PushM+B for sync.

Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of having (mostly) the same Push logic in more than 1 RPC, even if used only by the sync command. Once it's out there, it's an endpoint we'll probably have to maintain in case some customer invokes it manually. Already shared my FUD around doing this 😅 I won't repeat myself haha, I won't block if that's what you folks agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current patterns we have around this are based on where we're coming from, not where we're going. Pretty soon we'll allow tags to move, so push --tag <foo>, even with identical content, should move the tag to the latest commit. Internally that may call Latest + either Push or SetTag, but that RPC layout is suboptimal.

I think where we're headed is a world in which everyone uses buf sync exclusively and push --tag becomes obsolete. push would remain for bespoke/advanced use cases where you need to fall down to the primitive, and this would be used in cases where you're not syncing something, per se (like dynamically downloaded and pushed schemas, somewhat like managed modules).

@saquibmian saquibmian merged commit ad43253 into main May 31, 2023
@saquibmian saquibmian deleted the smian/syncservice branch May 31, 2023 20:23
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