-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Implement initial gRPC API #11224
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
Conversation
"services": { | ||
"grpc": { | ||
"address": ":9001", | ||
"tls": null |
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.
I think you should be able to omit this
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.
The Go struct that gets serialized to create this config is this one:
TLS *TLSConfiguration `json:"tls" yaml:"tls"` |
The TLS
field doesn't specify omitempty
so it's always unmarshalled to nil
.
I think the TLS
field should specify omitempty
, then we can get rid of this line as you suggest.
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.
Makes sense, wanna make that change?
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.
I'm going to do it in another PR because doing so will tag the workspace team into this one as reviewers which doesn't seem worth it.
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.
here: #11240
43a3092
to
2538d22
Compare
Add a service for the gRPC port exposed by the usage component.
Already implemented by the embedded struct.
74acb58
to
b681cc1
Compare
6ee0243
to
b681cc1
Compare
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.
Approving as self-hosted wrongly marked as owner on these files
@andrew-farries With this being "blocked" I have a hard time understanding what state this PR is in. 😕 It is ready to be merged (marked as "ready for review" + approved)? Also, I did not notice any reason for the |
I'm not sure what you mean by "blocked" here. The PR is ready to review. Only caveat is that ca861c2 was already reviewed in #11236 and 7b25556, 22f51dd were reviewed in #11240. Maybe stacked PRs here are causing more confusion than they are worth. I've removed the hold - there was no reason for it. I generally like to add it to PRs so I can be in control of when it gets merged eg if there are any "optional" changes required from reviewers. /unhold |
Personally, I'd recommend using stacked PRs as a Queue only and not to merge later PRs into their predecestor, it makes it easier to follow what needs reviewing. With that, I'd also keep any follow-on PRs as draft until they can actually be reviewed. |
Description
#11221 added the
.proto
definitions for a placeholder gRPC API for theusage
component.This PR implements the server side of that placeholder API.
Also updates the installer to deploy config for the
usage
component so that it serves gRPC and adds a k8s service through which it can be accessed.Related Issue(s)
Fixes #10324
How to test
usage
service locally:The placeholder API is shown:
Release Notes
Documentation
Werft options:
/hold