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

Jump to bottom

[storage][front-end] Add google.api.http #186

Merged
merged 3 commits into from
Nov 4, 2022
Merged

[storage][front-end] Add google.api.http #186

merged 3 commits into from
Nov 4, 2022

Conversation

glimchb
Copy link
Member

@glimchb glimchb commented Nov 2, 2022 •
edited
Loading

Following Google AIP-127 - HTTP and gRPC Transcoding

and https://cloud.google.com/endpoints/docs/grpc/transcoding

Related to #174

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

PWAlessi
PWAlessi approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@PWAlessi PWAlessi left a comment

Choose a reason for hiding this comment

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

other than the url paths/names, looks good.

PWAlessi
PWAlessi suggested changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@PWAlessi PWAlessi left a comment

Choose a reason for hiding this comment

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

url paths and names need to change

@glimchb
[storage] add missing google proto files
2cc1ea2 
See https://grpc-ecosystem.github.io/grpc-gateway/docs/tutorials/adding_annotations/#using-protoc

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb
[storage][front-end] Add google.api.http
9310927 
Following Google AIP-127 - HTTP and gRPC Transcoding

Related to opiproject#174

Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb
[storage] auto-generate protos
9b9f544 
Signed-off-by: Boris Glimcher <Boris.Glimcher@emc.com>
@glimchb glimchb marked this pull request as ready for review November 3, 2022 19:24
@glimchb glimchb requested a review from a team as a code owner November 3, 2022 19:24
@glimchb glimchb added Storage APIs or code related to storage area Merge Candidate in the open merge window, next candidate for merge labels Nov 3, 2022
@glimchb glimchb requested a review from a team November 3, 2022 19:25
@glimchb
Copy link
Member Author

glimchb commented Nov 3, 2022

url paths and names need to change

fixed

PWAlessi
PWAlessi approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@PWAlessi PWAlessi left a comment

Choose a reason for hiding this comment

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

I'm not positive that this is correct because I've never implemented protobuf over REST/http. I'd say that if the proto compiler works, it's probably fine to merge. I'd also add a backlog item for building out (at least a mock) http server to see if it works as expected.

@glimchb
Copy link
Member Author

glimchb commented Nov 3, 2022

I'm not positive that this is correct because I've never implemented protobuf over REST/http. I'd say that if the proto compiler works, it's probably fine to merge. I'd also add a backlog item for building out (at least a mock) http server to see if it works as expected.

opened #188 to track backlog item

jainvipin
jainvipin reviewed Nov 3, 2022
View reviewed changes
Copy link
Contributor

@jainvipin jainvipin left a comment

Choose a reason for hiding this comment

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

one comment @glimchb on the path hierarchy

storage/v1/frontend_nvme_pcie.proto Show resolved Hide resolved
storage/v1/frontend_virtio_blk.proto Show resolved Hide resolved
@glimchb glimchb mentioned this pull request Nov 3, 2022
Open
@llabordehpe
Copy link

Shouldn't the objects "hierarchy" be part of the the URLs (like in this example)?
For example, in NVMeControllerCreate, don't we want:
post: "/v1/subsystems/{subsystem}/controllers"
body: "controller"
instead of :
post: "/v1/controllers"
body: "controller"

@glimchb
Copy link
Member Author

glimchb commented Nov 4, 2022

Shouldn't the objects "hierarchy" be part of the the URLs (like in this example)? For example, in NVMeControllerCreate, don't we want: post: "/v1/subsystems/{subsystem}/controllers" body: "controller" instead of : post: "/v1/controllers" body: "controller"

Yes, it should. I just didn't want to overload this PR with hierarchy since it can lead to a heavy discussion on who is parent of who... I opened #188 to track the correct hierarchy fixes

@glimchb glimchb merged commit bb436ed into opiproject:main Nov 4, 2022
llabordehpe
llabordehpe reviewed Nov 4, 2022
View reviewed changes
storage/v1/frontend_nvme_pcie.proto
}
rpc NVMeSubsystemUpdate (NVMeSubsystemUpdateRequest) returns (NVMeSubsystem) {
option (google.api.http) = {
patch: "/v1/subsystems"
Copy link

Choose a reason for hiding this comment

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

I think it shoud have been:
patch: "/v1/subsystems/{subsystem}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, good catch!

@mkalderon
Copy link
Contributor

@glimchb late response. looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jainvipin jainvipin jainvipin left review comments

@llabordehpe llabordehpe llabordehpe left review comments

@mestery mestery mestery approved these changes

@PWAlessi PWAlessi PWAlessi approved these changes

@benlwalker benlwalker Awaiting requested review from benlwalker

@mkalderon mkalderon Awaiting requested review from mkalderon

@orendu orendu Awaiting requested review from orendu

Assignees
No one assigned
Labels
Merge Candidate in the open merge window, next candidate for merge Storage APIs or code related to storage area
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@glimchb @llabordehpe @mkalderon @mestery @PWAlessi @jainvipin