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

Handle buf warnings for streaming methods http rules #150

Open
avive opened this issue Jun 15, 2021 · 6 comments
Open

Handle buf warnings for streaming methods http rules #150

avive opened this issue Jun 15, 2021 · 6 comments

Comments

@avive
Copy link
Contributor

avive commented Jun 15, 2021

Currently, there are no http rules for the streaming methods. These rules are used to generate the gateway json api.
A bit of research is needed hre- can thegateway json api support Spacemesh streaming grpc methods? if yes - update the rules to include the streaming methods. If not - remove the build warnings about missing streaming methods.
@daonb @lrettig

@lrettig
Copy link
Member

lrettig commented Jun 20, 2021

The json gateway should be able to support the streaming endpoints. But I don't think we've ever tested this. I'm disinclined to add them since we should generally discourage use of the json gateway in favor of the underlying grpc endpoints.

@avive
Copy link
Contributor Author

avive commented Jun 20, 2021

Okay - so the question is why do we support json/http at all, even for the non-streaming endpoints?

@daonb
Copy link

daonb commented Jun 28, 2021

I've seen the CI code in the api branch and it does use the json streaming api but it's the wrong thing to do. It accesses the nodes through a curl instance running inside the namespace. We've all agreed that it's better for the test runner itself to run inside the namespace and use gRPC to access the nodes.

IMO, it's better to remove the json endpoints.

@avive
Copy link
Contributor Author

avive commented Jun 28, 2021

I see no reason to keep them in the node level for 0.3 and I agree that we should better remove them so no new code relies on them.

@lrettig
Copy link
Member

lrettig commented Jun 28, 2021

I don't have a strong opinion here. There's already support for the json gateway in the node, so I'm inclined to leave it there. Just have it off by default.

@avive
Copy link
Contributor Author

avive commented Jul 28, 2021

I think we should get rid of the http-json gateway altogether and this will resolve this issue. See #151

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

No branches or pull requests

3 participants