Skip to content

Commit

Permalink
Remove HandleReadModuleError (#2585)
Browse files Browse the repository at this point in the history 
The logic is instead inlined because it's a core decision that Syncer
should make.
  • Loading branch information
@saquibmian
saquibmian authored Nov 15, 2023
1 parent 99a4db9 commit 69c8e54
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 231 deletions.
103 changes: 0 additions & 103 deletions private/buf/bufsync/bufsync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,111 +31,8 @@ import (
// ErrModuleDoesNotExist is an error returned when looking for a BSR module.
var ErrModuleDoesNotExist = errors.New("BSR module does not exist")

const (
// ReadModuleErrorCodeModuleNotFound happens when the passed module directory does not have any
// module.
ReadModuleErrorCodeModuleNotFound = iota + 1
// ReadModuleErrorCodeUnnamedModule happens when the read module does not have a name.
ReadModuleErrorCodeUnnamedModule
// ReadModuleErrorCodeInvalidModuleConfig happens when the module directory has an invalid module
// configuration.
ReadModuleErrorCodeInvalidModuleConfig
// ReadModuleErrorCodeBuildModule happens when the read module errors building.
ReadModuleErrorCodeBuildModule
// ReadModuleErrorCodeUnexpectedName happens when the read module has a different name than
// expected, usually the one in the branch HEAD commit.
ReadModuleErrorCodeUnexpectedName
)

// ReadModuleErrorCode is the type of errors that can be thrown by the syncer when reading a module
// from a passed module directory.
type ReadModuleErrorCode int

// ReadModuleError is an error that happens when trying to read a module from a module directory in
// a git commit.
type ReadModuleError struct {
err error
code ReadModuleErrorCode
branch string
commit string
moduleDir string
}

// Code returns the error code for this read module error.
func (e *ReadModuleError) Code() ReadModuleErrorCode {
return e.code
}

// Code returns the module directory in which this error code was thrown.
func (e *ReadModuleError) ModuleDir() string {
return e.moduleDir
}

func (e *ReadModuleError) Error() string {
return fmt.Sprintf(
"read module in branch %s, commit %s, directory %s: %s",
e.branch, e.commit, e.moduleDir, e.err.Error(),
)
}

const (
// LookbackDecisionCodeSkip instructs the syncer to skip the commit that threw the read module
// error, and keep looking back.
LookbackDecisionCodeSkip = iota + 1
// LookbackDecisionCodeOverride instructs the syncer to use the read module and override its
// identity with the target module identity for that directory, read either from the branch's HEAD
// commit, or the passed module identity override in the command.
LookbackDecisionCodeOverride
// LookbackDecisionCodeStop instructs the syncer to stop looking back when finding the read module
// error, and use the previous commit (if any) as the start sync point.
LookbackDecisionCodeStop
// LookbackDecisionCodeFail instructs the syncer to fail the lookback process for the branch,
// effectively failing the sync process.
LookbackDecisionCodeFail
)

// LookbackDecisionCode is the decision made by the ErrorHandler when finding a commit that throws
// an error reading a module.
type LookbackDecisionCode int

// ErrorHandler handles errors reported by the Syncer before or during the sync process.
type ErrorHandler interface {
// HandleReadModuleError is invoked when navigating a branch from HEAD and seeing an error reading
// a module.
//
// For each branch to be synced, the Syncer travels back from HEAD looking for modules in the
// given module directories, until finding a commit that is already synced to the BSR, or the
// beginning of the Git repository.
//
// The syncer might find errors trying to read a module in that directory. Those errors are sent
// to this function to know what to do on those commits.
//
// decide if the Syncer should stop looking back or not, and choose the previous one (if any) as
// the start sync point.
//
// e.g.: The git commits in topological order are: `a -> ... -> z (HEAD)`, and the modules on a
// given module directory are:
//
// commit | module name or failure | could be synced? | why?
// ----------------------------------------------------------------------------------------
// z | buf.build/acme/foo | Y | HEAD
// y | buf.build/acme/foo | Y | same as HEAD
// x | buf.build/acme/bar | N | different than HEAD
// w | unnamed module | N | no module name
// v | unbuildable module | N | module does not build
// u | module not found | N | no module name, no 'buf.yaml' file
// t | buf.build/acme/foo | Y | same as HEAD
// s | buf.build/acme/foo | Y | same as HEAD
// r | buf.build/acme/foo | N | already synced to the BSR
//
// If this func returns `LookbackDecisionCodeSkip` for any `ReadModuleErrorCode`, then the syncer
// will stop looking when reaching the commit `r` because it already exists in the BSR, select `s`
// as the start sync point, and the synced commits into the BSR will be [s, t, x, y, z].
//
// If this func returns `LookbackDecisionCodeStop` 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].
HandleReadModuleError(err *ReadModuleError) LookbackDecisionCode
// InvalidBSRSyncPoint is invoked by Syncer upon encountering a module's branch sync point that is
// invalid locally. A typical example is either a sync point that points to a commit that cannot
// be found anymore, or the commit itself has been corrupted.
Expand Down
9 changes: 0 additions & 9 deletions private/buf/bufsync/bufsync_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,6 @@ func (c *testSyncHandler) setSyncPoint(branchName string, hash git.Hash, identit
branch.manualSyncPoint = hash
}

func (c *testSyncHandler) HandleReadModuleError(
readErr *bufsync.ReadModuleError,
) bufsync.LookbackDecisionCode {
if readErr.Code() == bufsync.ReadModuleErrorCodeUnexpectedName {
return bufsync.LookbackDecisionCodeOverride
}
return bufsync.LookbackDecisionCodeSkip
}

func (c *testSyncHandler) InvalidBSRSyncPoint(
identity bufmoduleref.ModuleIdentity,
branch string,
Expand Down
158 changes: 69 additions & 89 deletions private/buf/bufsync/syncer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ const (
LookbackTimeLimit = 24 * time.Hour
)

var (
errReadModuleInvalidModule = errors.New("invalid module")
errReadModuleInvalidModuleConfig = errors.New("invalid module config")
)

type syncer struct {
logger *zap.Logger
repo git.Repository
Expand Down Expand Up @@ -218,7 +223,7 @@ func (s *syncer) prepareSync(ctx context.Context) error {
var targetModuleIdentity bufmoduleref.ModuleIdentity
if identityOverride == nil {
// no identity override, read from HEAD
builtModule, readErr := s.readModuleAt(ctx, branch, headCommit, moduleDir)
builtModule, readErr := s.readModuleAt(ctx, headCommit, moduleDir)
if readErr != nil {
// any error reading module in HEAD, skip syncing that module in that branch
s.logger.Warn(
Expand All @@ -227,6 +232,15 @@ func (s *syncer) prepareSync(ctx context.Context) error {
)
continue
}
if builtModule == nil {
s.logger.Debug(
"no module on HEAD, skipping branch",
zap.String("branch", branch),
zap.String("moduleDir", moduleDir),
)
// no module on branch
continue
}
targetModuleIdentity = builtModule.ModuleIdentity()
} else {
// disregard module name in HEAD, use the identity override
Expand Down Expand Up @@ -437,34 +451,34 @@ func (s *syncer) branchSyncableCommits(
return git.ErrStopForEach
}
// git commit is not synced, attempt to read the module in the commit:moduleDir
builtModule, readErr := s.readModuleAt(
ctx, branch, commit, moduleDir,
readModuleAtWithExpectedModuleIdentity(targetModuleIdentity),
)
if readErr == nil {
commitsForSync = append(commitsForSync, &syncableCommit{commit: commit, module: builtModule})
return nil
builtModule, err := s.readModuleAt(ctx, commit, moduleDir)
if err != nil {
if errors.Is(err, errReadModuleInvalidModule) || errors.Is(err, errReadModuleInvalidModuleConfig) {
logger.Debug("read module at commit failed, skipping commit", zap.Error(err))
return nil
}
return err
}
decision := s.handler.HandleReadModuleError(readErr)
switch decision {
case LookbackDecisionCodeFail:
return fmt.Errorf("read module error: %w", readErr)
case LookbackDecisionCodeSkip:
logger.Debug("read module at commit failed, skipping commit", zap.Error(readErr))
if builtModule == nil {
logger.Debug("module not found, skipping commit")
return nil
case LookbackDecisionCodeStop:
logger.Debug("read module at commit failed, stop looking back in branch", zap.Error(readErr))
return git.ErrStopForEach
case LookbackDecisionCodeOverride:
logger.Debug("read module at commit failed, overriding module identity in commit", zap.Error(readErr))
if builtModule == nil {
return fmt.Errorf("cannot override commit, no built module: %w", readErr)
}
if builtModule.ModuleIdentity() == nil {
if _, hasOverride := s.modulesDirsToIdentityOverrideForSync[moduleDir]; !hasOverride {
logger.Debug("unnamed module, no override, skipping commit")
return nil
}
} else if builtModule.ModuleIdentity().IdentityString() != targetModuleIdentity {
if _, hasOverride := s.modulesDirsToIdentityOverrideForSync[moduleDir]; !hasOverride {
logger.Debug("module name doesn't match HEAD, no override, skipping commit")
return nil
}
// no need to rename the module, the module identity for this built module won't be used
commitsForSync = append(commitsForSync, &syncableCommit{commit: commit, module: builtModule})
return nil
}
return fmt.Errorf("unexpected decision code %d for read module error %w", decision, readErr)
commitsForSync = append(commitsForSync, &syncableCommit{
commit: commit,
module: builtModule,
})
return nil
}
if err := s.repo.ForEachCommit(
eachCommitFunc,
Expand Down Expand Up @@ -512,49 +526,30 @@ func (s *syncer) isGitCommitSynced(ctx context.Context, moduleIdentity bufmodule
return commitSynced, nil
}

// readModuleAt returns a module that has a name and builds correctly given a commit and a module
// directory, or a read module error. If the module builds, it might be returned alongside a not-nil
// error.
// readModule returns a module that has a name and builds correctly given a commit and a module
// directory.
func (s *syncer) readModuleAt(
ctx context.Context,
branch string,
commit git.Commit,
moduleDir string,
opts ...readModuleAtOption,
) (*bufmodulebuild.BuiltModule, *ReadModuleError) {
var readOpts readModuleOpts
for _, opt := range opts {
opt(&readOpts)
}
// in case there is an error reading this module, it will have the same branch, commit, and module
// dir that we can fill upfront. The actual `err` and `code` (if any) is populated in case-by-case
// basis before returning.
readModuleErr := &ReadModuleError{
branch: branch,
commit: commit.Hash().Hex(),
moduleDir: moduleDir,
}
) (*bufmodulebuild.BuiltModule, error) {
commitBucket, err := s.storageGitProvider.NewReadBucket(commit.Tree(), storagegit.ReadBucketWithSymlinksIfSupported())
if err != nil {
readModuleErr.err = fmt.Errorf("new read bucket: %w", err)
return nil, readModuleErr
return nil, fmt.Errorf("new read bucket: %w", err)
}
moduleBucket := storage.MapReadBucket(commitBucket, storage.MapOnPrefix(moduleDir))
foundModule, err := bufconfig.ExistingConfigFilePath(ctx, moduleBucket)
if err != nil {
readModuleErr.err = fmt.Errorf("looking for an existing config file path: %w", err)
return nil, readModuleErr
return nil, fmt.Errorf("looking for an existing config file path: %w", err)
}
if foundModule == "" {
readModuleErr.code = ReadModuleErrorCodeModuleNotFound
readModuleErr.err = errors.New("module not found")
return nil, readModuleErr
// No module at this commit.
return nil, nil
}
sourceConfig, err := bufconfig.GetConfigForBucket(ctx, moduleBucket)
if err != nil {
readModuleErr.code = ReadModuleErrorCodeInvalidModuleConfig
readModuleErr.err = fmt.Errorf("invalid module config: %w", err)
return nil, readModuleErr
// Invalid config.
return nil, fmt.Errorf("%w: %s", errReadModuleInvalidModuleConfig, err)
}
builtModule, err := bufmodulebuild.NewModuleBucketBuilder().BuildForBucket(
ctx,
Expand All @@ -563,25 +558,7 @@ func (s *syncer) readModuleAt(
bufmodulebuild.WithModuleIdentity(sourceConfig.ModuleIdentity),
)
if err != nil {
readModuleErr.code = ReadModuleErrorCodeBuildModule
readModuleErr.err = fmt.Errorf("build module: %w", err)
return nil, readModuleErr
}
// module builds, unnamed and unexpectedName errors can be returned alongside the built module.
if sourceConfig.ModuleIdentity == nil {
readModuleErr.code = ReadModuleErrorCodeUnnamedModule
readModuleErr.err = errors.New("found module does not have a name")
return builtModule, readModuleErr
}
if readOpts.expectedModuleIdentity != "" {
if sourceConfig.ModuleIdentity.IdentityString() != readOpts.expectedModuleIdentity {
readModuleErr.code = ReadModuleErrorCodeUnexpectedName
readModuleErr.err = fmt.Errorf(
"read module has an unexpected module identity %s, expected %s",
sourceConfig.ModuleIdentity.IdentityString(), readOpts.expectedModuleIdentity,
)
return builtModule, readModuleErr
}
return nil, fmt.Errorf("%w: %s", errReadModuleInvalidModule, err)
}
return builtModule, nil
}
Expand Down Expand Up @@ -619,14 +596,27 @@ func (s *syncer) backfillTags(
if len(tagsToBackfill) == 0 {
return nil
}
// For each older commit we travel, we need to make sure it's a valid module with the expected
// module identity, or that the error handler would have chosen to override it.
if _, readErr := s.readModuleAt(
ctx, branch, oldCommit, moduleDir,
readModuleAtWithExpectedModuleIdentity(moduleIdentity.IdentityString()),
); readErr != nil && s.handler.HandleReadModuleError(readErr) != LookbackDecisionCodeOverride {
// read module failed, and the error handler would not have overwritten it
// For each older commit we travel, we need to make sure it's a valid module with a module
// identity or an overridden module identity.
if builtModule, err := s.readModuleAt(ctx, oldCommit, moduleDir); err != nil {
if errors.Is(err, errReadModuleInvalidModule) || errors.Is(err, errReadModuleInvalidModuleConfig) {
// read module failed, skip commit
return nil
}
return fmt.Errorf("read module at commit %q in dir %q: %w", oldCommit, moduleDir, err)
} else if builtModule == nil {
// no module, skip commit
return nil
} else if builtModule.ModuleIdentity() == nil {
if _, hasOverride := s.modulesDirsToIdentityOverrideForSync[moduleDir]; !hasOverride {
logger.Debug("unnamed module, no override, skipping tagged commit")
return nil
}
} else if builtModule.ModuleIdentity().IdentityString() != moduleIdentity.IdentityString() {
if _, hasOverride := s.modulesDirsToIdentityOverrideForSync[moduleDir]; !hasOverride {
logger.Debug("module name doesn't match HEAD, no override, skipping tagged commit")
return nil
}
}
logger := logger.With(
zap.String("commit", oldCommit.Hash().Hex()),
Expand All @@ -651,16 +641,6 @@ func (s *syncer) backfillTags(
return nil
}

type readModuleOpts struct {
expectedModuleIdentity string
}

type readModuleAtOption func(*readModuleOpts)

func readModuleAtWithExpectedModuleIdentity(moduleIdentity string) readModuleAtOption {
return func(opts *readModuleOpts) { opts.expectedModuleIdentity = moduleIdentity }
}

// printSyncPreparation prints information gathered at the sync preparation step.
func (s *syncer) printSyncPreparation() {
s.logger.Debug(
Expand Down
1 change: 0 additions & 1 deletion private/buf/cmd/buf/command/alpha/repo/reposync/reposync.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@ func sync(
container,
repo,
createWithVisibility,
modulesDirsWithOverrides,
),
syncerOptions...,
)
Expand Down
Loading

0 comments on commit 69c8e54

Please sign in to comment.