-
Notifications
You must be signed in to change notification settings - Fork 566
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
[PFS-230] Initial fileset service protos #9893
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Protos look fine.
While I don't think there is any specific rule here, this is the first proto file not named the same as the directory it's in; i.e. normally this would be storage.proto
.
I think you need to do two more things here:
-
Run Gazelle to generate the go build rules for this new package (
bazel run //:gazelle
). -
Export protos from this package so the builder can see them by adding this rule to the newly-generated src/storage/BUILD.bazel file:
filegroup(
name = "protos",
srcs = glob(["*.proto"]),
visibility = ["//src:__pkg__"],
)
- Add this new filegroup to src/BUILD.bazel's
all_protos
filegroup.
(Basically if there is no BUILD file in the directory, we can glob the *.protos for make_proto... but after you run Gazelle so you can use the generated code from Go, then the glob won't work anymore and you have to explicitly select the protos.)
Also need "bazel run //:buildifier"; the list of deps for the all_protos target should be sorted and that tool will do it for you. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9893 +/- ##
==========================================
- Coverage 58.31% 58.21% -0.10%
==========================================
Files 605 605
Lines 73341 73341
==========================================
- Hits 42766 42696 -70
- Misses 30018 30090 +72
+ Partials 557 555 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, however, I'd like to know if this is intended to be customer facing/available through the SDK. Although these changes always generate protos, if they are within a new grpc service then the INT teams needs to manually expose these within the client. So if/when these are intended to be user-facing let us know and we'll expose them
This PR contains the initial protos for the new fileset service described here: https://www.notion.so/2024-02-09-Filesets-gRPC-Service-10f40b1eec9a4af8b48100aadb710fc9
This PR contains the initial fileset service implementation, as specified here: #9893
This PR contains the initial protos for the new fileset service described here: https://www.notion.so/2024-02-09-Filesets-gRPC-Service-10f40b1eec9a4af8b48100aadb710fc9