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

Revamp CreateCommits to just create Commits and add specialized Push and Rebase endpoints #43

Merged
merged 25 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ BIN := .tmp/bin
export PATH := $(BIN):$(PATH)
export GOBIN := $(abspath $(BIN))

BUF_VERSION := 21ba19590be4afe6fa3af0406e138c4fdc36fedf
BUF_VERSION := v1.28.1
COPYRIGHT_YEARS := 2023

.PHONY: help
Expand Down
8 changes: 4 additions & 4 deletions buf/registry/module/v1beta1/branch.proto
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ message Branch {
// Only one Branch per module will be marked as the release Branch.
// TODO: enum?
bool is_release = 9 [(buf.validate.field).required = true];
// The id of the latest Commit created on the Branch.
string latest_commit_id = 10 [
// The id of the head Commit created on the Branch.
string head_commit_id = 10 [
(buf.validate.field).required = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this is required. Does this imply that an empty Branch does not exist, or should we relax this constraint?

Copy link
Member

Choose a reason for hiding this comment

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

Unless this causes a problem elsewhere, it makes sense to me that an empty branch doesn't exist because this is true in Git (a branch pointer has to point to a commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the docs to reflect this.

(buf.validate.field).string.uuid = true
];
// The Digest of the latest Commit pushed to the Branch.
buf.registry.storage.v1beta1.Digest latest_commit_digest = 11 [(buf.validate.field).required = true];
// The Digest of the head Commit pushed to the Branch.
buf.registry.storage.v1beta1.Digest head_commit_digest = 11 [(buf.validate.field).required = true];
}

// BranchRef is a reference to a Branch, either an id or a fully-qualified name.
Expand Down
146 changes: 146 additions & 0 deletions buf/registry/module/v1beta1/branch_service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,23 @@ service BranchService {
rpc ListBranches(ListBranchesRequest) returns (ListBranchesResponse) {
option idempotency_level = NO_SIDE_EFFECTS;
}
// Push existing Commits onto Branches.
//
// This operation is atomic. Either all Commits are pushed to the referenced Branches in the order
// provided or an error is returned.
//
// This operation does not allow the caller to assert the state of Branches before pushing any
// Commits onto them. As a result, multiple concurrent PushCommits operations that target the same
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
// Branch will all succeed, and the head Commit on the Branch will be determined by the last
// request that succeeded. To deterministically push Commits onto a Branch, use Rebase instead.
rpc Push(PushRequest) returns (PushResponse);
// Rebase existing Commits onto Branches.
//
// This operation is atomic. Either all Branches are rebased as represented in the given Operations,
// or an error is returned. Additionally, each update in the request asserts the expected head Commit
// of the branch. If the actual head Commit does not match the expected head Commit, all Operations
// are rejected and an error is returned.
rpc Rebase(RebaseRequest) returns (RebaseResponse);
}

message GetBranchesRequest {
Expand Down Expand Up @@ -98,3 +115,132 @@ message ListBranchesResponse {
// The listed Branches.
repeated Branch branches = 2;
}

message PushRequest {
message Value {
// The Branch to push the Commits to.
//
// If this is a name reference, and the named Branch does not exist, it will
// be created and the given Commits will be the first Commits on the Branch.
BranchRef branch_ref = 1 [(buf.validate.field).required = true];
// The ID of the Commits to push to the Branch, in the order they should be added.
//
// This Commit will become the head Commit on the Branch.
bufdev marked this conversation as resolved.
Show resolved Hide resolved
//
// If the Commit already exists on the Branch, an error is returned.
bufdev marked this conversation as resolved.
Show resolved Hide resolved
repeated string commit_ids = 2 [
(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.items = {
string: {uuid: true}
}
];
}
// The Commits to push with the Branches they should be pushed to.
//
// All Values should have unique BranchRefs, that is no two Values should have
// a BranchRef that refers to the same Branch. An error will be returned if any
// two BranchRefs refer to the same Branch.
repeated Value values = 1 [(buf.validate.field).repeated.min_items = 1];
}
Copy link
Member

Choose a reason for hiding this comment

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

@saquibmian do we need to support pushing to multiple branches simultaneously - push is last writer wins anyways, so maybe it's not worthwhile? If we got rid of that, we'd be able to simplify this RPC and get rid of the nested Value message entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not, but not supporting batching is inconsistent with the rest of the schema. Every RPC today supports batching, except the List* RPCs (which in theory could support batching trivially).


message PushResponse {
// The Branches that were pushed to, in the order they appeared in the request.
repeated Branch branches = 1 [(buf.validate.field).repeated.min_items = 1];
}

message RebaseRequest {
// An operation to apply as part of the rebase.
message Operation {
// An operation to create a Branch.
message CreateBranch {
// The Module to create the Branch on.
ModuleRef module_ref = 1 [(buf.validate.field).required = true];
// The name of the Branch to create.
//
// If a Branch with this name already exists on the Module, this will result in an error.
string branch_name = 2 [
(buf.validate.field).required = true,
(buf.validate.field).string.max_len = 250
];
// The ID of the Commit for which to start history from. The history of the branch will
bufdev marked this conversation as resolved.
Show resolved Hide resolved
// reflect the history as if you called ListCommitHistory for this Commit ID.
//
// If this is empty, the created Branch will only have the Commits specified in
// add_commit_ids.
//
// This is primarily designed for use in forking a Branch instead of
// providing the new Branch's entire history.
string start_commit_id = 3 [(buf.validate.field).string.uuid = true];
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
// The IDs of the Commits to add to the Branch, in the order they should be added.
//
// If start_commit_id is specified, these will be appended to the Commit history
// that aleady exists up to start_commit_id.
//
// If start_commit_id is not specified, the newly-created Branch will have exactly
// these Commits, in this order.
repeated string add_commit_ids = 4 [(buf.validate.field).repeated.items = {
string: {uuid: true}
}];
}

// An operation to update an existing branch.
message UpdateBranch {
// The branch to update.
BranchRef branch_ref = 1 [(buf.validate.field).required = true];
// The ID of the Commit that the caller expects to be the head Commit on the Branch.
//
// If this ID does not match the ID of the head Commit on the Branch, an error is
// returned.
string expected_head_commit_id = 2 [
(buf.validate.field).required = true,
(buf.validate.field).string.uuid = true
];
// The IDs of the Commits to remove from the Branch.
//
// Commits are removed before any Commits are added from add_commit_ids.
//
// Note that it is valid to have one of the remove_commit_ids be equal to the
// expected_head_commit_id. If this is the case, the Branch will have a different
// head Commit that add_commit_ids will be applied to.
repeated string remove_commit_ids = 3 [(buf.validate.field).repeated.items = {
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
string: {uuid: true}
}];
// The IDs of the Commits to add to the Branch, in the order they should be added.
//
// These are added starting at the head Commit after remove_commit_ids has been applied.
//
// Note that is is valid to have IDs in both add_commit_ids and remove_commit_ids. If
// this is the case, the IDs will be removed from the Branch prior to re-adding them after
// the head Commit via add_commit_ids.
repeated string add_commit_ids = 4 [(buf.validate.field).repeated.items = {
string: {uuid: true}
}];
}

oneof value {
option (buf.validate.oneof).required = true;
// Create a new branch.
CreateBranch create_branch = 1;
// Update an existing branch.
UpdateBranch patch_branch = 2;
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
}
}

// The operations to apply.
//
// The Operations will be applied in the order they are provided. Notably, this means that
// if a CreateBranch Operation precedes a UpdateBranch Operation for the same Branch, the
// UpdateBranch Operation is applied to the newly-created Branch, including any new Commits
// that were added from the CreateBranch Operation. Conversely, if a UpdateBranch Operation
// precedes the CreateBranch operation that creates the Branch, this will result in an error.
repeated Operation operations = 1 [(buf.validate.field).repeated.min_items = 1];
}

message RebaseResponse {
// The Branches that were updated. This list will be the unique set of Branches
// that were updated, and the length of values will be less than the length
// of the input Operations if multiple Operations were applied for a single Branch.
// However, the response Branches ordered in the same order that the Branches
// first appeared in the request Operations.
repeated Branch branches = 1 [(buf.validate.field).repeated.min_items = 1];
}
62 changes: 27 additions & 35 deletions buf/registry/module/v1beta1/commit_service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,10 @@ service CommitService {
}
// Create commits on a Module with associated Content.
//
// This is used by push and sync.
// These commits will not be associated with any Branch or Tag after the completion of
// this operation. To associate created Commits, use BranchService.Push or BranchService.Rebase.
//
// This operation is atomic. Either all Commits and associated content are created or an error is returned.
//
// TODO: PushCommits? Something else? This is creating potentially a bunch of resources.
rpc CreateCommits(CreateCommitsRequest) returns (CreateCommitsResponse) {
option idempotency_level = IDEMPOTENT;
}
Expand Down Expand Up @@ -79,9 +78,11 @@ message ResolveCommitsRequest {
// - If a Commit is referenced, this just references that specific commit.
// - If a Tag is referenced, this is interpreted to mean the Commit associated with the Tag.
// - If a VCSCommit is referenced, this is interpreted to mean the Commit associated with the VCSCommit.
// - Is a Branch is referenced, this is interpreted to mean the latest Commit on the Branch.
// - Is a Branch is referenced, this is interpreted to mean the head Commit on the Branch.
// - If a Digest is referenced, this is interpreted to mean the latest released Commit that has this Digest.
// Digests referencing unreleased Commits cannot be referenced.
//
// TODO: what is "latest released Commit" if we now have "head Commit"?
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to re-evaluate everywhere in this API we use the word "released" given our new buf sync model

Is "released" a property of a BSR commit?

  • If a BSR commit can be on multiple branches, then I don't think we can make released a property on a BSR commit (because "released" only makes sense within the context of a history).
  • If a BSR commit can only ever be on a single branch, they it would maybe be ok to model it this way

Also need to take a look at branches service and probably rename references to "release branch" to "default branch"

Copy link
Member

Choose a reason for hiding this comment

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

Yes we need to re-evaluate this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something we can do separate from this PR, or do we want to formalize "released" now and rope it into this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should try to land this PR and save this bigger TODO for a followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, I'll leave this here for now and we can clean up/formalize "release" after this.

repeated ResourceRef resource_refs = 1 [
(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
Expand Down Expand Up @@ -111,9 +112,11 @@ message ListCommitHistoryRequest {
// - If a Commit is referenced, history is started at this Commit.
// - If a Tag is referenced, history is started at the Commit associated with the Tag.
// - If a VCSCommit is referenced, history is started at the Commit associated with the VCSCommit.
// - Is a Branch is referenced, history is started at the latest Commit on the Branch.
// - Is a Branch is referenced, history is started at the head Commit on the Branch.
// - If a Digest is referenced, history is started at the latest released Commit that has this Digest.
// Digests referencing unreleased Commits cannot be referenced.
//
// TODO: what is "latest released Commit" if we now have "head Commit"?
ResourceRef resource_ref = 3 [(buf.validate.field).required = true];
// Only return Commits that have one or more associated Tags.
bool has_tag = 4;
Expand Down Expand Up @@ -193,41 +196,28 @@ message CreateCommitsRequest {
repeated DepNode dep_nodes = 3;
}

// The pointers to the content for the Modules that should have Commits created for them.
//
// Each ModuleNode must have a unique ModuleRef.
// A commit will be created for each ModuleNode.
repeated ModuleNode module_nodes = 1 [(buf.validate.field).repeated.min_items = 1];
// Blobs for the FileNodes referenced by module_nodes that are not present on the server.
message Value {
bufdev marked this conversation as resolved.
Show resolved Hide resolved
// The pointers to the content for the Modules that should have Commits created for them.
//
// Each ModuleNode must have a unique ModuleRef.
// A commit will be created for each ModuleNode.
repeated ModuleNode module_nodes = 1 [(buf.validate.field).repeated.min_items = 1];
// Associated VCS commit information.
//
// If there are already VCSCommits on the associated Module with a given hash, this
// will result in an error. Otherwise, a new VCSCommit is created.
repeated AssociatedVCSCommit associated_vcs_commits = 2;
}
// The requests to create Commits.
repeated Value values = 1 [(buf.validate.field).repeated.min_items = 1];
// Blobs for the FileNodes referenced by file_nodes that are not present on the server.
//
// Only Blobs that were returned as missing from GetMissingBlobDigests need to be sent.
// Other Blobs already exist on the server, and will be ignored.
//
// If a FileNode within module_nodes has a Blob that is not on the server, and is not
// If a FileNode has a Blob that is not on the server, and is not
// within missing_blobs, an error will be returned.
repeated buf.registry.storage.v1beta1.Blob missing_blobs = 2;
// The names of Branches that should be associated with this Commit.
//
// If a Branch currently exists on the associated Module with a name, this existing
// Branch will be used. Otherwise, a new Branch will be created for the corresponding name.
//
// If empty, the default branch is assumed as the only branch.
repeated string branch_names = 3 [(buf.validate.field).repeated.items = {
string: {max_len: 250}
}];
// The names of Tags that should be associated with this Commit.
//
// If a Tag currently exists on the assocated Module with a name, the RPC will error, however
// this will change in the future when we allow Tags to move. If the Tag does not
// currently exist, a new Tag will be created for each name.
repeated string tag_names = 4 [(buf.validate.field).repeated.items = {
string: {max_len: 250}
}];
// Associated VCS commit information.
//
// If there are already VCSCommits on the associated Module with a given hash, this
// will result in an error. Otherwise, a new VCSCommit is created.
repeated AssociatedVCSCommit associated_vcs_commits = 5;
}

message CreateCommitsResponse {
Expand All @@ -250,9 +240,11 @@ message GetCommitNodesRequest {
// - If a Commit is referenced, files are returned from this Commit.
// - If a Tag is referenced, files are returned from the Commit associated with the Tag.
// - If a VCSCommit is referenced, files are returned from the Commit associated with the VCSCommit.
// - Is a Branch is referenced, files are returned from the latest Commit on the Branch.
// - Is a Branch is referenced, files are returned from the head Commit on the Branch.
// - If a Digest is referenced, files are returned from the latest released Commit that has this Digest.
// Digests referencing unreleased Commits cannot be referenced.
//
// TODO: what is "latest released Commit" if we now have "head Commit"?
ResourceRef resource_ref = 1 [(buf.validate.field).required = true];
// Specific file paths to retrieve.
//
Expand Down
Loading