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.

Sign up for GitHub

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

Fix missing blobs request/response #31

Merged
merged 7 commits into from
Nov 11, 2023
Merged

Fix missing blobs request/response #31

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2941277
Fix missing blobs request/response
saquibmian Nov 9, 2023
7a1ef7b
Add validation rules
saquibmian Nov 9, 2023
3e3209e
s/module/module_ref
saquibmian Nov 9, 2023
6f7366f
fix type
saquibmian Nov 9, 2023
112ead6
Missing validate on request
saquibmian Nov 9, 2023
397eb9e
Switch to using `Value`
saquibmian Nov 10, 2023
8f800d1
Also fix GetBlobs
bufdev Nov 11, 2023
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
23 changes: 17 additions & 6 deletions buf/registry/module/v1beta1/commit_service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,14 +310,25 @@ message GetBlobsResponse {
}

message GetMissingBlobDigestsRequest {
// The digests to see if we have Blobs for.
repeated buf.registry.storage.v1beta1.Digest digests = 1 [
(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
];
message ModuleBlobDigests {
// The reference to the Module that the digests belong to.
ModuleRef module = 1;
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
// The digests to see if we have Blobs for.
repeated buf.registry.storage.v1beta1.Digest blob_digests = 2 [
(buf.validate.field).repeated.min_items = 1,
(buf.validate.field).repeated.max_items = 250
];
}
repeated ModuleBlobDigests module_blob_digests = 1;
saquibmian marked this conversation as resolved.
Show resolved Hide resolved
}

message GetMissingBlobDigestsResponse {
message MissingBlobDigest {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a single digest, this is a set of Digests, so this name doesn't make as much sense. I would want to call it ModuleDigests, but then the field repeated ModuleDigests module_digests doesn't really make sense, it's more like repeated ModuleDigests module_digests_es. Plurals for message names rarely make sense.

Perhaps something like ModuleBlobDigestSet, and then repeated ModuleBlobDigestSet module_blob_digest_sets, or just Value and repeated Value values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// The reference to the Module that the missing digests are for.
ModuleRef module = 1;
Copy link
Contributor Author

@saquibmian saquibmian Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@bufdev what type would I use here? ModuleRef is marked as request_only.

Is this canonical?

  string module_id = 6 [
    (buf.validate.field).required = true,
    (buf.validate.field).string.uuid = true
  ];

If so, that's incongruent with ModuleRef, which may not be an ID. Ideally the caller doesn't have to make other calls to fetch IDs for the modules (if that's the case, why take in ModuleRef at all).

Copy link
Member

Choose a reason for hiding this comment

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

It should be Module module = 1 on the response. Good catch.

Copy link
Member

Choose a reason for hiding this comment

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

and required

// The digests that are missing.
repeated buf.registry.storage.v1beta1.Digest missing_blob_digests = 2;
}
// The digests that are missing.
repeated buf.registry.storage.v1beta1.Digest missing_blob_digests = 1;
repeated MissingBlobDigest missing_blob_digests = 1;
}
Loading