this module loads the identity.proto file and creates a client for the Identity service.
Depends on D5573
Details
Details
imported the identityClient in keyserver.js and successfully called the getUserPublicKey RPC
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/grpc/grpc_client.js | ||
|---|---|---|
| 16 ↗ | (On Diff #18291) | in a subsequent diff, i'll introduce config that lets us toggle between localhost and the production hostname |
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 10–14 ↗ | (On Diff #18380) | According to https://www.npmjs.com/package/@grpc/proto-loader it looks that
const options = {
keepCase: true,
longs: String,
enums: String,
defaults: true,
oneofs: true
}And it makes me wonder what grpc.load is and if we can use it instead of introducing new library. Especially that we need to provide some configuration to it. Are we sure that these config options are correct for us? Do they affect the objects on client's side, or the data that is sent? |
| 18 ↗ | (On Diff #18380) | We definitely need a place where it can be stored and configured. |
| 19 ↗ | (On Diff #18380) | Why do we use something that isn't secure? What's the difference? |
Comment Actions
I plan to address config for endpoints and SSL in a subsequent diff. @tomek lmk if you think that needs to precede this diff
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 10–14 ↗ | (On Diff #18380) | grpc.load was a method in the now-deprecated C-based gRPC npm package. From the docs: If you are currently loading .proto files using grpc.load, that function is not available in this library. You should instead load your .proto files using @grpc/proto-loader and load the resulting package definition objects into @grpc/grpc-js using grpc.loadPackageDefinition. |
| 18 ↗ | (On Diff #18380) | yeah, I think I mentioned this in a previous version of this diff. a config file will be added in a subsequent diff |
| 19 ↗ | (On Diff #18380) | this should probably be configurable. createInsecure creates a channel that does not use SSL, which is what we want when we run the Identity service locally. createSsl will need to be used to talk to the production service. |
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 19 ↗ | (On Diff #18380) | If you agree something should be done, but want to land the diff anyways, you should always link a Linear task tracking that work |
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 19 ↗ | (On Diff #18380) | |
Comment Actions
I don't think it matters that much, so feel free to take whatever approach you like.
There's a new dependency included, so adding @ashoat as a blocking reviewer. (for me the dependency looks fine)
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 8–17 ↗ | (On Diff #18380) | Just wondering if this shouldn't be included in e.g. a function instead of being directly executed. Not sure if the current approach has significant drawbacks though. |
| 10–14 ↗ | (On Diff #18380) | Thanks for explaining! |
| 18 ↗ | (On Diff #18380) | Haven't seen that comment because it was made when a file had other name - but that makes sense |
| keyserver/src/grpc/grpc-client.js | ||
|---|---|---|
| 8–17 ↗ | (On Diff #18380) | The current approach is what I saw in other projects, I think it should be fine |