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.

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

Update callers of GetBufLockFileForPrefix to pass a resolve function #2681

Merged
merged 9 commits into from
Dec 21, 2023

Conversation

oliversun9
Copy link
Contributor

This is a followup to 39f0510, passing the BufLockFileWithDigestResolver option whenever possible.

  • Add helper to bufmoduleapi:
func CommitIDToDigest(
	ctx context.Context,
	clientProvider bufapi.ClientProvider,
	remote string,
	commitID string,
) (bufcas.Digest, error)
  • Pass a client provider to callers of GetBufLockFileForPrefix. These callers can then pass
func(ctx context.Context, remote string, commitID string) (bufcas.Digest, error) {
	return bufmoduleapi.CommitIDToDigest(ctx, p.clientProvider, remote, commitID)
},

as the resolve function.

In the future, to make fewer network calls, we could have a CommitResolver and create it with

NewCommitResolver(..., clientProvider, hints)

where hints are potential commit IDs to resolve.

@@ -65,6 +65,7 @@ func testBasic(t *testing.T, subDirPath string) {
zap.NewNop(),
tracing.NopTracer,
bucket,
nil,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels awkward to just pass a nil, should I create some NewNopClientProvider helper?

Copy link
Member

Choose a reason for hiding this comment

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

yea


tar bool
}

func newModuleDataStore(
logger *zap.Logger,
clientProvider bufapi.ClientProvider,
Copy link
Member

Choose a reason for hiding this comment

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

I would actually not pass it to the moduleDataStore, and instead add a comment that we expect all lock files in the moduleDataStore to have digests. We control this.

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

Mostly looks good, this was less ugly than I thought, see two comments (add a no-op ClientProvider, do not pass to ModuleDataStore)

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

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

I would add a comment in ModuleDataStore about why we are not passing the option as well.

//
// Should be used for testing only.
func NewNopClientProvider() ClientProvider {
return newNopClientProvider()
Copy link
Member

Choose a reason for hiding this comment

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

Nit: In other places, we just do var NopClientProvider ClientProvider = nopClientProvider{} as a global variable, and then you don't have any constructors, and you implement the functions directly on the struct, not the struct pointer, ie func (nopClientProvider) BranchServiceClient...

@oliversun9 oliversun9 merged commit 18f9c9f into bufmod Dec 21, 2023
3 of 7 checks passed
@oliversun9 oliversun9 deleted the osun/read-lock-without-digest branch December 21, 2023 05:28
bufdev added a commit that referenced this pull request Dec 27, 2023
This PR makes update to the migrate command:
* moves it from `buf beta migrate` to `buf migrate`
* Update flags to take `--module` and `--workspace` instead of paths to
those files.
* The destination for the buf.yaml and buf.lock is `.`, unless
`--workspace` is specified but `--module` is not specified, in which
case the destination is the directory for that `buf.yaml`.
- If the user runs `buf migrate --workspace foo/bar`, then the new
`buf.yaml` will be written at `foo/bar/buf.yaml`.
  - Otherwise, `./buf.yaml` will be written.
* Only allows one workspace to be specified. (This isn't new in this PR,
but want to highlight this).
* Add flag `--template`, allowing multiple paths to buf.gen.yamls.
* Fixes the bug where the command crashes when only one directory
without a buf.yaml is specified.
* Update lint/breaking rule translation to produce equivalent lists of
rules with minimal diff.

Note: with the recent change on [reading a lock file without
digest](#2681). Right now `buf
migrate` fails with `Failure: unimplemented` if there is a buf.lock to
write. It shouldn't fail once commit service has been implemented.

---------

Co-authored-by: bufdev <bufdev-github@buf.build>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants