This is almost completely copy-pasted from the work I did on the native rust library.
All the differences will be annotated inline.
Details
Details
imported the registerUser function and successfully called it from keyserver.js
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- arcpatch-D5796 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Hmm, if we need protobufs to be installed for yarn cleaninstall, seems like we should find a way to run scripts/install_protobuf.sh ahead of the Rust build via some Yarn script instead of patching in scripts/install_protobuf.sh ahead of every call we can find
| .buildkite/eslint_flow_jest.yml | ||
|---|---|---|
| 7 ↗ | (On Diff #20943) | The dependencies we install with apt vary from workflow to workflow, so I didn't move this into the install_protobuf script. e.g. |
| .buildkite/eslint_flow_jest.yml | ||
|---|---|---|
| 7 ↗ | (On Diff #20943) | oops forgot to finish this comment. meant to say, "e.g. on github CI cmake is already installed" |
Comment Actions
This is almost completely copy-pasted from the work I did on the native rust library.
In this case, maybe this diff isn't the best place to do any refactors, but In my opinion, the logic of this code is a bit difficult to follow. Some communication with the server (tx.send()) is handled directly in register_user() (and their results are then sent to helpers) while other parts are spread across helper functions and performed inside them. Or e.g. request is sent inside a helper fn, then response is handled in the outer fn. Or it might be my subjective feeling because these functions are not defined in order they're called.
| keyserver/addons/rust-node-addon/src/identity_client.rs | ||
|---|---|---|
| 78–81 ↗ | (On Diff #20951) | Can send_to_mpsc be used here? |
| 237–240 ↗ | (On Diff #20951) | Same here |
Comment Actions
this is super valid, but i think the refactor should be in a separate diff that addresses the logic here as well as in the native rust library. https://linear.app/comm/issue/ENG-2733/refactor-native-rust-library-and-rust-node-addon-client-logic
| keyserver/Dockerfile | ||
|---|---|---|
| 56 ↗ | (On Diff #20951) | I don't think this is right. ubuntu will bringing protoc 3.12 which we don't want. Should probably use shared/install_protobuf.sh script instead. |
| keyserver/addons/rust-node-addon/install_external_deps.sh | ||
| 3 ↗ | (On Diff #20951) | generally want this |
| 8–12 ↗ | (On Diff #20951) | |
| 15–16 ↗ | (On Diff #20951) | command return non-zero if it didn't find anything. >/dev/null if to don't want command outputting the command path if something is found: $ command -v kubectl /home/jon//.nix-profile/bin/kubectl |
| 18 ↗ | (On Diff #20951) | make the script CWD independent (e.g. ./keyserver/addons/rust-node-addon/install_external_deps.sh). Also, I would avoid source scripts when possible. Things like exit will then affect the sourcing script. |
Comment Actions
The CI stuff looks good once we swap out apt installation of protobuf-compiler with install_protobuf script.
| keyserver/Dockerfile | ||
|---|---|---|
| 56 ↗ | (On Diff #20951) | +1, would be great to use the install_protobuf script across the board. |
Comment Actions
Trust the CI, but can't patch on this diff from the latest master:
Cherry Pick Failed! Exception Command failed with error #1! COMMAND git cherry-pick -- arcpatch-D6261 STDOUT On branch arcpatch-D5796 You are currently cherry-picking commit fc1f8987b.
Comment Actions
No, just arc patch. Running yarn arcpatch or git pull --all --tags before arc patch solves the issue. Thanks!
| keyserver/addons/rust-node-addon/build.rs | ||
|---|---|---|
| 5–6 ↗ | (On Diff #20951) | Maybe better to place this compilation stage in the library's shared/comm-opaque/build.rs neither than in the keyserver's build.rs file. |
Comment Actions
address feedback
| keyserver/addons/rust-node-addon/build.rs | ||
|---|---|---|
| 5–6 ↗ | (On Diff #20951) | yeah i went back and forth on whether to have the shared library handle the tonic stuff too... i think the bindings for cxx and napi-rs are different enough that it doesn't make sense. can revisit this later |
Comment Actions
rename install_external_deps to install_ci_deps to make it clear what we're using the script for
| .github/workflows/eslint_flow_jest.yml | ||
|---|---|---|
| 15 | Since this script requires sudo on the GitHub CI machines, I think we should call it directly rather than from a different script | |
| keyserver/addons/rust-node-addon/install_ci_deps.sh | ||
| 5 | See other inline comment for explanation of why we're omitting GitHub CI here | |
| scripts/install_protobuf.sh | ||
| 26 | We should clean up the protobuf directory after we've installed it successfully | |
| keyserver/addons/rust-node-addon/postinstall.sh | ||
|---|---|---|
| 11 | Nit: get rid of this extra newline | |
| keyserver/addons/rust-node-addon/postinstall.sh | ||
|---|---|---|
| 11 | oops good catch | |