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

feat: added grpcweb proxy that allows me to access grpc endpoints from the ui #39

Merged
merged 9 commits into from
Jul 10, 2023

Conversation

Peeeekay
Copy link
Contributor

@Peeeekay Peeeekay commented Jul 6, 2023

Using grpcWeb proxy to enable ui to access grpc endpoints; here is the https://github.com/improbable-eng/grpc-web

To expose both grpcWeb and grpc, I am using cmux here: https://github.com/soheilhy/cmux/tree/master

Changelog picked up from commits here:

feat: added grpcweb proxy that allows me to access grpc endpoints from the ui on the same port

@Peeeekay Peeeekay requested a review from gbouv July 6, 2023 05:05
golang/go.mod Show resolved Hide resolved
golang/go.mod Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Show resolved Hide resolved
@Peeeekay Peeeekay requested a review from h4ck3rk3y July 7, 2023 14:30
@Peeeekay Peeeekay closed this Jul 7, 2023
@Peeeekay Peeeekay reopened this Jul 7, 2023
Copy link
Contributor

@h4ck3rk3y h4ck3rk3y left a comment

Choose a reason for hiding this comment

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

have asked some questions. pending concerns

  1. cmux hasn't had any updates since 2021 - is this still the right choice?
  2. some possible race conditions that I am worried about
  3. concerns about grpcweb not supporting a few of our features
  4. can I get a notion explaining this architecture. why we are making this choice? how will it be working underneath? how does cmux play with grpcweb? The next dev we onboard and the rest of the team can use this.

golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Outdated Show resolved Hide resolved
golang/server/server.go Show resolved Hide resolved
golang/server/server.go Show resolved Hide resolved
golang/server/server.go Show resolved Hide resolved
@Peeeekay
Copy link
Contributor Author

Peeeekay commented Jul 7, 2023

have asked some questions. pending concerns

  1. cmux hasn't had any updates since 2021 - is this still the right choice?
  2. some possible race conditions that I am worried about
  3. concerns about grpcweb not supporting a few of our features
  4. can I get a notion explaining this architecture. why we are making this choice? how will it be working underneath? how does cmux play with grpcweb? The next dev we onboard and the rest of the team can use this.

1.cmux for multiplexes seems like the best choice at least for serving requests on same port; this is not long term imo. I have seen it being used by other repos like opentracing etc.. so don't think it should be a huge issue.

  1. let me check the race condition query you had - will get back on that.

3.grpcweb not supporting features like bi-di and client is fine for now; as in near term that is not needed and by then we should invest time on supporting connect.

  1. Sure; though I don't think this is long term solution -- this can be seen as stopgap solution.

@h4ck3rk3y
Copy link
Contributor

Is it okay for the frontend to not support upload files / upload StarlarkPackage?

@Peeeekay
Copy link
Contributor Author

Peeeekay commented Jul 7, 2023

Is it okay for the frontend to not support upload files / upload StarlarkPackage?

yeah - i don;t think we are supporting that behaviour in short term for frontend.

@Peeeekay Peeeekay merged commit 90b674a into main Jul 10, 2023
@Peeeekay Peeeekay deleted the pk/add_grpcweb_proxy branch July 10, 2023 16:42
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.

3 participants