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.

Sign up for GitHub

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

Jump to bottom

Remove HandleReadModuleError #2585

Merged
merged 7 commits into from
Nov 15, 2023
Merged

Remove HandleReadModuleError #2585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b402d15
Remove `HandleReadModuleError`
saquibmian Nov 13, 2023
8e6a6f3
Handle nil module
saquibmian Nov 13, 2023
a6d0bd0
Kill unused LookbackDecisionCode
saquibmian Nov 13, 2023
772d8dd
Merge branch 'main' into saquib/bsr-3037-remove-handlereadmoduleerror
saquibmian Nov 14, 2023
b5682b1
Missing case
saquibmian Nov 14, 2023
ecc7ad8
One more spot
saquibmian Nov 14, 2023
c6ef6dd
pr comments
saquibmian Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 0 additions & 103 deletions private/buf/bufsync/bufsync.go
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

How is this error different than the ones when reading a module? Do you see this being customizable by some CLI flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah I want to kill this, too. Initially all of this came because we were logging a warning in the CLI and the suggestion was made that Syncer shouldn't do this, but this is a core part of Syncer now.

// 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
Open in desktop
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
Open in desktop
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")
unmultimedio marked this conversation as resolved.
Show resolved Hide resolved
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 {
unmultimedio marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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