-
Notifications
You must be signed in to change notification settings - Fork 289
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.
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
Change CheckSyncedGitCommits
to IsGitCommitSynced
#2561
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -425,7 +425,7 @@ func (s *syncer) branchSyncableCommits( | |
commitHash := commit.Hash().Hex() | ||
logger := logger.With(zap.String("commit", commitHash)) | ||
// check if this commit is already synced | ||
isSynced, err := s.isGitCommitSynced(ctx, moduleIdentity, commitHash) | ||
isSynced, err := s.isGitCommitSynced(ctx, moduleIdentity, commit.Hash()) | ||
if err != nil { | ||
return fmt.Errorf( | ||
"checking if module %s already synced git commit %s: %w", | ||
|
@@ -516,26 +516,25 @@ func (s *syncer) branchSyncableCommits( | |
} | ||
|
||
// isGitCommitSynced checks if a commit hash is already synced to a BSR module. | ||
func (s *syncer) isGitCommitSynced(ctx context.Context, moduleIdentity bufmoduleref.ModuleIdentity, commitHash string) (bool, error) { | ||
func (s *syncer) isGitCommitSynced(ctx context.Context, moduleIdentity bufmoduleref.ModuleIdentity, commitHash git.Hash) (bool, error) { | ||
modIdentity := moduleIdentity.IdentityString() | ||
// check local cache first | ||
if syncedModuleCommits, ok := s.modulesIdentitiesToCommitsSyncedCache[modIdentity]; ok { | ||
if _, commitSynced := syncedModuleCommits[commitHash]; commitSynced { | ||
if _, commitSynced := syncedModuleCommits[commitHash.Hex()]; commitSynced { | ||
return true, nil | ||
} | ||
} | ||
// not in the cache, request BSR check | ||
syncedModuleCommits, err := s.handler.CheckSyncedGitCommits(ctx, moduleIdentity, map[string]struct{}{commitHash: {}}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We only ever passed in one hash. |
||
commitSynced, err := s.handler.IsGitCommitSynced(ctx, moduleIdentity, commitHash) | ||
if err != nil { | ||
return false, err | ||
} | ||
_, commitSynced := syncedModuleCommits[commitHash] | ||
if commitSynced { | ||
// populate local cache | ||
if s.modulesIdentitiesToCommitsSyncedCache[modIdentity] == nil { | ||
s.modulesIdentitiesToCommitsSyncedCache[modIdentity] = make(map[string]struct{}) | ||
} | ||
s.modulesIdentitiesToCommitsSyncedCache[modIdentity][commitHash] = struct{}{} | ||
s.modulesIdentitiesToCommitsSyncedCache[modIdentity][commitHash.Hex()] = struct{}{} | ||
} | ||
return commitSynced, nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,7 +31,6 @@ import ( | |
"github.com/bufbuild/buf/private/pkg/connectclient" | ||
"github.com/bufbuild/buf/private/pkg/git" | ||
"github.com/bufbuild/buf/private/pkg/storage" | ||
"github.com/bufbuild/buf/private/pkg/stringutil" | ||
"go.uber.org/zap" | ||
"google.golang.org/protobuf/types/known/timestamppb" | ||
) | ||
|
@@ -88,30 +87,22 @@ func (h *syncHandler) ResolveSyncPoint(ctx context.Context, module bufmoduleref. | |
return hash, nil | ||
} | ||
|
||
func (h *syncHandler) CheckSyncedGitCommits(ctx context.Context, module bufmoduleref.ModuleIdentity, commitHashes map[string]struct{}) (map[string]struct{}, error) { | ||
func (h *syncHandler) IsGitCommitSynced(ctx context.Context, module bufmoduleref.ModuleIdentity, hash git.Hash) (bool, error) { | ||
service := connectclient.Make(h.clientConfig, module.Remote(), registryv1alpha1connect.NewLabelServiceClient) | ||
res, err := service.GetLabelsInNamespace(ctx, connect.NewRequest(®istryv1alpha1.GetLabelsInNamespaceRequest{ | ||
RepositoryOwner: module.Owner(), | ||
RepositoryName: module.Repository(), | ||
LabelNamespace: registryv1alpha1.LabelNamespace_LABEL_NAMESPACE_GIT_COMMIT, | ||
LabelNames: stringutil.MapToSlice(commitHashes), | ||
LabelNames: []string{hash.Hex()}, | ||
})) | ||
if err != nil { | ||
if connect.CodeOf(err) == connect.CodeNotFound { | ||
// Repo is not created | ||
return nil, nil | ||
} | ||
return nil, fmt.Errorf("get labels in namespace: %w", err) | ||
} | ||
syncedHashes := make(map[string]struct{}) | ||
for _, label := range res.Msg.Labels { | ||
syncedHash := label.LabelName.Name | ||
if _, expected := commitHashes[syncedHash]; !expected { | ||
return nil, fmt.Errorf("received unexpected synced hash %q, expected %v", syncedHash, commitHashes) | ||
return false, nil | ||
} | ||
syncedHashes[syncedHash] = struct{}{} | ||
return false, fmt.Errorf("get labels in namespace: %w", err) | ||
} | ||
return syncedHashes, nil | ||
return len(res.Msg.Labels) == 1, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be make sure that the returned label is actually the one requested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this, but this is going away soon anyway, to be replaced by |
||
} | ||
|
||
func (h *syncHandler) GetModuleReleaseBranch(ctx context.Context, module bufmoduleref.ModuleIdentity) (string, error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I remember when doing this I was imagining paging many git hashes in a single request, but with multi module directories that became increasingly difficult and right now we just do it one by one. I'm good with this change!