[keyserver] try to get identity config from file if env var is not present
ClosedPublic
Actions

Authored by varun on Dec 2 2023, 3:57 PM.

Details

Reviewers

bartek
ashoat

Commits

rCOMMaac431ec1679: [keyserver] try to get identity config from file if env var is not present

Summary

Depends on D10147

keep the env var logic in place for the Docker workflow, but now also try to get the JSON from a file for normal yarn dev workflow

Test Plan

tried both Docker and yarn dev workflows, saw the expected logs and confirmed that the socket address was the fake one i set in the config

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

varun created this revision.Dec 2 2023, 3:57 PM

Herald added a subscriber: tomek. · View Herald TranscriptDec 2 2023, 3:57 PM

Harbormaster completed remote builds in B24718: Diff 34166.Dec 2 2023, 4:24 PM

varun requested review of this revision.Dec 2 2023, 4:24 PM

Can we do this in a more general way, similar to how getCommConfig works? Eg. factor out the "env var or config file" logic

Otherwise defer on the Rust

In D10148#295111, @ashoat wrote:

Can we do this in a more general way, similar to how getCommConfig works? Eg. factor out the "env var or config file" logic

Agreed, the getCommConfig functionality could be easily implemented in Rust. But not sure if it's worth it for just this single value. This can be introduced later when needed (but we need a task, neither ENG-4196 nor ENG-4885 are accurate enough).

This revision is now accepted and ready to land.Dec 3 2023, 11:00 PM

How hard is it to implement such a simple function? If this was JS, it would take 10-15min or so, and I would do it now. Not sure if Rust somehow makes this more difficult?

In D10148#295222, @ashoat wrote:

How hard is it to implement such a simple function? If this was JS, it would take 10-15min or so, and I would do it now. Not sure if Rust somehow makes this more difficult?

Might be some effort with static variables (for caching), we still use old lazy_static crate here which has some limitations, instead of newer once_cell.
Other parts should be straightforward (e.g. extracting the "env-var vs file fallback" you mentioned) so you're right it's better doing it now

Returning this back to @varun

keyserver/addons/rust-node-addon/src/identity_client/mod.rs
67–70	When extracting the function for reading config, I'd also make it generic and include `serde_json::from_str<T>()` inside it

This revision now requires changes to proceed.Dec 4 2023, 6:07 AM

address feedback

varun added inline comments.Dec 4 2023, 1:03 PM

keyserver/addons/rust-node-addon/src/identity_client/config.rs
55 ↗	(On Diff #34199)	i think this is sufficient error handling for now, but lmk what you think @bartek
keyserver/addons/rust-node-addon/src/identity_client/mod.rs
39 ↗	(On Diff #34199)	idk why this was `pub`, but it shouldn't be

Harbormaster completed remote builds in B24736: Diff 34199.Dec 4 2023, 1:17 PM

ashoat mentioned this in D10201: b[native] configure socket address on native with json file.Dec 6 2023, 12:53 AM

bartek accepted this revision.Dec 6 2023, 2:30 AM

bartek added inline comments.

keyserver/addons/rust-node-addon/src/identity_client/config.rs
40–51 ↗	(On Diff #34199)	Optionally these could be methods of `impl ConfigName`, that would be more elegant than C-style func calls
55 ↗	(On Diff #34199)	yeah it's ok