Sync a single module by default, at the root of the repo #2330

unmultimedio · 2023-08-01T17:02:40Z

This PR sets a much saner default behavior and sync rules when running buf alpha repo sync:

Don't force users to pass --module flag each time for all module dirs. If they don't, then assume a single module, at the root of the git repo.
Read module identities from the HEAD commit for each branch, and only sync commits in the branch history that match those names.
Skip commits with read module failures like:
- Module not found
- Module with invalid config
- Module unnamed
- Module name different than HEAD
- Module does not build
Improve sync output every time a git commit is synced to a new more readable format

…nch-rpcs

…sync

…a/sync-single-branch

…-branch

…lt-behavior

…t-behavior

unmultimedio · 2023-08-03T23:06:21Z

private/buf/bufsync/bufsync.go

-}
-
-// Module is a module that will be synced by Syncer.
-type Module interface {


Since --module flag no longer has the format of --module <module-dir>:<remote-module-identity-override>, but just --module <module-dir>, this was not needed anymore.

unmultimedio · 2023-08-03T23:08:53Z

private/buf/bufsync/bufsync.go

-	// Identity is the identity of the module, accounting for any configured override.
-	Identity() bufmoduleref.ModuleIdentity
-	// Bucket is the bucket for the module.
-	Bucket() storage.ReadBucket
-	// Commit is the commit that the module is sourced from.
-	Commit() git.Commit
 	// Branch is the git branch that this module is sourced from.
 	Branch() string
+	// Commit is the commit that the module is sourced from.
+	Commit() git.Commit
 	// Tags are the git tags associated with Commit.
 	Tags() []string
+	// Directory is the directory relative to the root of the git repository that this module is
+	// sourced from.
+	Directory() string
+	// Identity is the identity of the module.
+	Identity() bufmoduleref.ModuleIdentity
+	// Bucket is the bucket for the module.
+	Bucket() storage.ReadBucket


Sorted by visit order:

branch > commit(tags) > directory > module (identity, bucket)

and added directory.

unmultimedio · 2023-08-03T23:14:49Z

private/buf/bufsync/commits_to_sync.go

+			return stopLoopErr
+		}
+		commitHash := commit.Hash().Hex()
+		modulesDirsToSyncInThisCommit := make(map[string]bufmodulebuild.BuiltModule)


We will hold the built module in here, so we read each module on each commit just once, instead of reading it here and again while syncing.

unmultimedio · 2023-08-03T23:27:04Z

private/buf/bufsync/error.go

+	// On the other hand, if this func returns true for `ReadModuleErrorCodeModuleNotFound`, the
+	// syncer will stop looking when reaching the commit `u`, will select `v` as the start sync point,
+	// and the synced commits into the BSR will be [x, y, z].
+	StopLookback(err *ReadModuleError) bool


This way, if we want in the future, a flag or config could control in which scenarios we stop looking back, and which ones we skip.

unmultimedio · 2023-08-03T23:29:13Z

private/buf/cmd/buf/command/alpha/repo/reposync/reposync.go

-	Modules          []string
+	ModulesDirs      []string


You could say this is the main trigger for the whole PR. We are no longer receiving module identities overrides, but reading it from HEAD on each branch, for each module directory.

unmultimedio

Style comments

private/buf/bufsync/commits_to_sync.go

private/buf/bufsync/error.go

private/buf/bufsync/preparation.go

unmultimedio

Moved types functions to its file, mainly syncer.go and bufsync.go. Renamed a few types, mainly maps to reflect keyToValue, and changed some return []type to []*type.

unmultimedio · 2023-08-04T22:09:53Z

private/buf/bufsync/syncer.go

-	if syncPoint == nil {
-		return nil, nil
+	if s.moduleDefaultBranchGetter == nil {
+		s.logger.Warn(


I refrained myself of doing logger.Sugar().Warn( since the output seemed to be more confusing:

With sugar:

bufl alpha repo sync --timeout 0 --all-branches \ --module module-a \ --module module-b \ --create --create-visibility public ... WARN read module from HEAD failed, module won't be synced for this branch{error 26 0 read module in branch newcontent, commit 890b1d857a468e788feea6c34c26b7e2168ed75d, directory module-b: module not found} WARN default branch validation skipped{default_git_branch 15 0 main <nil>} {module 15 0 bufbuild.internal/jfigueroa/module-a <nil>} {error 26 0 BSR module does not exist} ...

Without sugar:

bufl alpha repo sync --timeout 0 --all-branches \ --module module-a \ --create --create-visibility public ... WARN read module from HEAD failed, module won't be synced for this branch {"error": "read module in branch newcontent, commit 890b1d857a468e788feea6c34c26b7e2168ed75d, directory module-a: module not found"} WARN default branch validation skipped {"default_git_branch": "main", "module": "bufbuild.internal/jfigueroa/module-a", "error": "BSR module does not exist"} ...

private/buf/bufsync/commits_to_sync.go

private/buf/bufsync/module_reader.go

unmultimedio · 2023-08-04T22:24:51Z

private/buf/bufsync/syncer.go

+	syncAllBranches          bool
+
+	commitsToTags                   map[string][]string                               // commits:[]tags
+	branchesToModulesForSync        map[string]map[string]bufmoduleref.ModuleIdentity // branch:moduleDir:moduleIdentityInHEAD


Used the suffix *ForSync instead of *ToSync in many variables in this pkg so it wouldn't be confused with the To that separates keys and values in maps, like in this one.

doriable

This looks good to me. I know there are some outstanding discussions on UX, but I believe those should be addressed in a follow-up PR. 🚀 🎉

doriable · 2023-08-08T15:33:02Z

private/buf/cmd/buf/command/alpha/repo/reposync/reposync.go

-			"Only modules specified via '--module' are synced.",
+			"Syncing all branches is possible using '--all-branches' flag. " +
+			"By default a single module at the root of the repository is assumed, " +
+			"for specific module paths use the '--module' flag. " +


nit: we can do this in a follow-up, but we should provide a more detailed example of how the --module flag is used.

Sure, although that I think it's already addressed in the --module flag docs. I'll check and add it in upcoming PRs if it's not there.

unmultimedio added 26 commits July 27, 2023 15:26

Repo's default branch RPCs

8d7bb89

Fix repo service mock

3a28aad

Add audit event

60f2551

Remove new RPCs in favor of embedding into repo

90b29bc

Merge remote-tracking branch 'origin/main' into jfigueroa/default-bra…

e00c1ea

…nch-rpcs

Remove RPCs from mock

95bcfba

Remove "git" prefix to default branch in repository

f47d647

Add default branch validation step

e6a71bb

Improve default branch validation

a048356

Fix linter

766c06d

Single branch default behavior

32adbca

s/baseBranch/defaultBranch/

37b9139

Use the current branch in syncer

3bc0737

Get current branch

ab7017d

Test current branch

6d03ca8

Pass ctx to open repository

0760c7f

Error on detached head

fbb1d3d

Fix check remote presence

648d6c8

Remove unnecessary func

5c3bfdd

Merge branch 'main' into jfigueroa/make-sure-default-branches-are-in-…

ec3b151

…sync

Merge commit 'ec3b15163c9f707776f97c863d247dbe2fa63072' into jfiguero…

d2e2796

…a/sync-single-branch

Initialize module identities from values in HEAD

931ed26

Validate default branches fixes

050bff4

Merge remote-tracking branch 'origin/main' into jfigueroa/sync-single…

28cef8c

…-branch

Fix cmd description

c05c734

Merge branch 'jfigueroa/sync-single-branch' into jfigueroa/sync-defau…

4df2c53

…lt-behavior

Base automatically changed from jfigueroa/sync-single-branch to main August 1, 2023 17:42

unmultimedio added 3 commits August 1, 2023 12:43

WIP Keep order of modules

f62865f

Merge remote-tracking branch 'origin/main' into jfigueroa/sync-defaul…

5804d7a

…t-behavior

WIP module target

2d2f6c1

unmultimedio added 7 commits August 3, 2023 16:46

No need to clear first or last commit

47d2401

Improve error message

5f69dd8

Merge remote-tracking branch 'origin/main' into jfigueroa/sync-defaul…

03a6064

…t-behavior

Warn if nothing to sync

4496b55

Clean up not needed syncableModule

457310a

Fix test

0a4b86b

Fix comments and wording

ef734cb

unmultimedio requested review from bufdev, doriable, seankimdev and a team August 3, 2023 23:03

unmultimedio marked this pull request as ready for review August 3, 2023 23:03

unmultimedio added 3 commits August 3, 2023 18:07

Rename option func

bd1e408

Extract copy of pending modules for branch

c69d127

More renames

1329a40

unmultimedio commented Aug 3, 2023

View reviewed changes

unmultimedio commented Aug 4, 2023

View reviewed changes

unmultimedio added 3 commits August 4, 2023 16:37

Move files into its type file

b91a49e

More renames

8ac2a59

Move errors on top

1b51480

unmultimedio commented Aug 4, 2023

View reviewed changes

mv util funcs to syncer

cfa5916

bufdev approved these changes Aug 4, 2023

View reviewed changes

private/buf/bufsync/commits_to_sync.go Outdated Show resolved Hide resolved

private/buf/bufsync/commits_to_sync.go Outdated Show resolved Hide resolved

private/buf/bufsync/module_reader.go Outdated Show resolved Hide resolved

Make expected identity optional

7822c96

unmultimedio commented Aug 4, 2023

View reviewed changes

unmultimedio added 2 commits August 4, 2023 17:29

Make option not error

34e18c3

format

d959df8

doriable approved these changes Aug 8, 2023

View reviewed changes

unmultimedio merged commit 25da2f0 into main Aug 8, 2023
7 checks passed

unmultimedio deleted the jfigueroa/sync-default-behavior branch August 8, 2023 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync a single module by default, at the root of the repo #2330

Sync a single module by default, at the root of the repo #2330

unmultimedio commented Aug 1, 2023 •

edited

unmultimedio Aug 3, 2023

unmultimedio Aug 3, 2023 •

edited

unmultimedio Aug 3, 2023

unmultimedio Aug 3, 2023

unmultimedio Aug 3, 2023

unmultimedio left a comment

unmultimedio left a comment

unmultimedio Aug 4, 2023

unmultimedio Aug 4, 2023

doriable left a comment

doriable Aug 8, 2023

unmultimedio Aug 8, 2023

Sync a single module by default, at the root of the repo #2330

Sync a single module by default, at the root of the repo #2330

Conversation

unmultimedio commented Aug 1, 2023 • edited

Choose a reason for hiding this comment

unmultimedio Aug 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unmultimedio left a comment

Choose a reason for hiding this comment

unmultimedio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doriable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unmultimedio commented Aug 1, 2023 •

edited

unmultimedio Aug 3, 2023 •

edited