Implement buf alpha repo sync
#2122
Conversation
Only base branch is supported today. `sync` replicates pushed git state to the BSR, walking commits in order. New or moved tags are also pushed.
|
Alright, this one is a rough draft, it's just to get it out there for initial assessment. |
|
|
||
| const ( | ||
| errorFormatFlagName = "error-format" | ||
| moduleFlagName = "module" |
There was a problem hiding this comment.
Why not first arg, similar to all other commands?
There was a problem hiding this comment.
It can be repeated, unless we're cool with repeated inputs?
There was a problem hiding this comment.
You could make the same arguments for most of our commands (buf lint could take multiple inputs), but there's no reason to, and deviating from that standard without a clear reason to do so makes the UX inconsistent. Is there a clear reason to do so here?
| &f.Modules, | ||
| moduleFlagName, | ||
| nil, | ||
| "The module(s) to sync to the BSR; you can provide a module override in the format "+ |
There was a problem hiding this comment.
I would expect sync to be congruent with other commands, namely taking a module or workspace as first arg
There was a problem hiding this comment.
Please don't resolve comments that are not resolved.
| ) error { | ||
| // Assume that this command is run from the repository root. If not, `OpenRepository` will return | ||
| // a dir not found error. | ||
| repo, err := git.OpenRepository(git.DotGitDir, command.NewRunner()) |
There was a problem hiding this comment.
THIS feels like a flag that defaults to .
There was a problem hiding this comment.
I explicitly don't want to let the user control the .git dir, I want them to run sync from the root of the repo they are syncing. The fewer options they can configure, the simpler the command is to use.
There was a problem hiding this comment.
Comments should be resolved by the creator when there is a design disagreement
| return err | ||
| } | ||
|
|
||
| func pushOrCreate( |
There was a problem hiding this comment.
This looks copied from push - worth making into a common command in bufcli? Or will this diverge?
There was a problem hiding this comment.
It's 90% a copy, yeah, namely the the message sent as part of PushManifestAndBlobs is different. I would definitely like to move this to a bufpush package.
There was a problem hiding this comment.
This has diverged, calling an entirely different RPC now.
…buf-alpha-repo-sync-command
private/buf/bufsync/syncer.go
Outdated
| // We failed to build the module. We can warn on this | ||
| // and carry on. Note that because of resumption, we will typically only come | ||
| // across this commit once, we will not log this warning again. | ||
| s.logger.Warn( |
There was a problem hiding this comment.
This is something that is conceivably part of control flow - it is conceivable that the caller of bufsync will want to know that a module was not built, and take some type of action. By logging here, we've effectively swallowed the warning. Instead, such data should be returned up the stack in a manner that allows the caller to decide what to do (including potentially log it)
private/buf/bufsync/syncer.go
Outdated
| // We found a module but the module config is invalid. We can warn on this | ||
| // and carry on. Note that because of resumption, we will typically only come | ||
| // across this commit once, we will not log this warning again. | ||
| s.logger.Warn( |
private/buf/bufsync/bufsync.go
Outdated
|
|
||
| // NewModule constructs a new module that can be synced with a Syncer. | ||
| func NewModule(dir string, identityOverride bufmoduleref.ModuleIdentity) (Module, error) { | ||
| path, err := normalpath.NormalizeAndValidate(dir) |
There was a problem hiding this comment.
I should move this into newSyncableModule. Let me do it up.
saquibmian
deleted the
saquib/bsr-1894-implement-buf-alpha-repo-sync-command
branch
Only base branch is supported today.
syncreplicates pushed git state to the BSR, walking commits in order. New or moved tags are also pushed.