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

Make federation a first-class citizen #16

Closed
wants to merge 1 commit into from

Conversation

bufdev
Copy link
Member

@bufdev bufdev commented Nov 5, 2023

This PR makes federation a first-class citizen by effectively adding the concept of registry_hostname to all resources. registry_hostname is a rename of what we currently call remote within our codebases.

Motivation:

  • There's work that I want to do to no longer send the full buf.yaml over the wire to the backend - we're defining the buf.yaml to not make up part of a Module, and once you do this, it becomes somewhat easy to deconstruct what you send over the wire. Even moreso, I'd argue this is desirable - the backend should not have to concern itself with buf.yamls, their layout, or their parsing, as they will not make up part of a Commit's digest.
  • To do such deconstruction, you need some message that makes up what we call, ModulePin right now - a Commit and its associated Digest. However, both historically and likely soon-to-be in the future, we've allowed for federation, meaning that it's possible that some `Commits' may lie outside of the given BSR instance you are calling via the API.
  • You therefore need a way to represent the registry of a Commit both in the request, and returned Commit objects.
  • It was pointed out that a shortcoming of this API is that federation wasn't accounted for, and this is a use case we currently have for legacy reasons, regardless of what we want going forward (which may be federation as well).

The path to this PR:

  • First, you realize you need to add a registry_hostname to Commit. Easy enough.
  • Next, you realize that you need to qualify all IDs on the Commit as scoped to the BSR instance as denoted by registry_hostname. For example, module_id is now the ID of the Module on the BSR instance as denoted by registry_hostname.
  • Inevitably, this means you need registry_hostnames the whole way down - Modules, Users, Organizations specifically.
  • At this point, for consistency and because we're letting half of our resources account for federation, you realize you likely want registry_hostname on Tag, VCSCommit, Branch as well.

What this PR does:

  • Adds registry_hostname to all resources.
  • Clarifies in the comments that IDs are scoped to the given registry_hostname.
  • Refactors .*Refs: .*FullNames are moved inside Refs as a nested message Name (this is likely a nice cleanup anyways, as we only use FullNames inside of Refs anyways at the API level). Since both id and name must be scoped to a registry_hostname, it is not desirable to repeat registry_hostname on both a theoretical id wrapper, and .*FullName, hence now defining .*FullName as a nested message that requires other data in the encapsulating message.
  • Adds a CommitRef that will be used in the CommitService on CreateCommits in a future PR.
  • Updates basically every comment to say that registry_hostnames need to match the BSR instance you are invoking for now. As this changes, we can relax these comments.

To be honest, I don't like it. It feels very messy. But I don't know if there's a better solution here, assuming we need to account for federation.

Let's discuss this on Monday @rodaine @jhump @Alfus @pkwarren @jchadwick-buf @saquibmian @doriable @mfridman and come to a conclusion.

//
// A Name uniquely identifies a Branch within a BSR instance.
// This is used for requests when a caller only has the branch name and not the ID.
message Name {
Copy link

@Alfus Alfus Nov 6, 2023

Choose a reason for hiding this comment

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

(nit: might "read" better to have Branch.Name instead of BranchRef.Name, which also makes me wonder about Branch.Ref)

Copy link

@Alfus Alfus Nov 6, 2023

Choose a reason for hiding this comment

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

(which then also makes me wonder about using Branch.Name inside Branch, instead of flattening the fields into the main message) (oops, those are ids vs names huh?)

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't flatten them because it's in a oneof

Copy link

Choose a reason for hiding this comment

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

(I was suggesting unflattening the owner_id, etc, but now I realize those are ids not names)

@bufdev
Copy link
Member Author

bufdev commented Nov 6, 2023

Closing as we've decided not to move forward with representing federation as-is in the new API, and will evaluate how and when we want to handle federation in the future, likely moving concerns client-side.

@bufdev bufdev closed this Nov 6, 2023
bufdev added a commit that referenced this pull request Nov 6, 2023
…d module and dependency content (#20)

This replaces #17 as #16 was closed.

This removes the sending of `buf.yaml` and `buf.lock` files over the
wire, and consequently these being part of the structure of `Modules` as
we know them, instead sending over the specific information we need on
the backend in a structured manner.

This fully standardizes on the `Node` terminology. A `Node` is a pointer
to some content, either on the request or response side. Within this API
now, we have:

- `FileNode`: A pointer to file content.
- `CreateCommitRequest.ModuleNode` - A set of `FileNodes` and an
associated `ModuleRef` that represents the content of a single
`Module.`.
- `CreateCommitRequest.DepNode` - A set of pointers to dependencies,
which are just the `commit_ids` of the dependencies and their associated
`Digest` (for verification).
- `CreateCommitResponse.CommitNode` - A set of pointers to files and
then actual commit dependencies.

This renames `GetFileNodes` to `GetCommitNodes`.

To be honest, I don't love the `Node` naming, and I think it could use
some work, but that's less important for the purpose of this PR. We can
work on that. Ideas I've had:

- `Info` for new nested types, ie `ModuleInfo, DepInfo, CommitInfo`.
- `Data` for new nested types. In general, I think it's an anti-pattern
to name a Protobuf message `.*Data`, as all Protobuf messages are data,
and it doesn't pluralize well.

Open to other suggestions.
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