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

Move DepRefs and DigestType to top-level #76

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Move DepRefs and DigestType to top-level #76

merged 3 commits into from
Feb 23, 2024

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Feb 23, 2024

This PR's primary purpose is to remove the need to specify dependencies between Contents on Upload, and to move external dependencies to the top-level of the UploadRequest. We do not need to manually specify dependencies between Contents, as this is implicitly done via .proto imports. We also want to make sure that for a given set of Contents, there is exactly one dependency Commit ID for a given Module. This matches what we have in Workspaces. The API will no longer enable uploading multiple Workspaces in one RPC, but we see no use case for this.

This PR also moves DigestType to the top-level of DownloadRequest, to match the other RPCs.

//
// Commits should be unique by Module, that is no two dep_refs should have the same Module but
// different Commit IDs.
repeated DepRef dep_refs = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it an error to include a DepRef that the BSR prunes away? I.e., if a Commit is referenced here that is never imported (even transitively) by any Content.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not an error I would say

Co-authored-by: Saquib Mian <[email protected]>
@bufdev bufdev merged commit c559234 into main Feb 23, 2024
6 checks passed
@bufdev bufdev deleted the top-level branch February 23, 2024 19:51
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.

3 participants