Page MenuHomePhabricator

[services] Tunnelbroker - Load dev-mode config file on start
ClosedPublic

Authored by max on May 25 2022, 1:38 PM.
Tags
None
Referenced Files
F3380255: D4130.diff
Wed, Nov 27, 10:32 PM
Unknown Object (File)
Sun, Nov 17, 1:39 AM
Unknown Object (File)
Thu, Nov 14, 4:39 PM
Unknown Object (File)
Thu, Nov 14, 4:39 PM
Unknown Object (File)
Oct 28 2024, 3:03 AM
Unknown Object (File)
Oct 28 2024, 3:03 AM
Unknown Object (File)
Oct 28 2024, 3:03 AM
Unknown Object (File)
Oct 28 2024, 3:03 AM

Details

Summary

Load dev-mode config file instead of default on service dev-mode start.

Linear task: ENG-1101

Test Plan

Service load tunnelbroker.ini config file on default start using yarn run-tunnelbroker-service command.
Service load dev-mode tunnelbroker-dev.ini config file on dev-mode start using run-tunnelbroker-service-dev-mode command.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

max held this revision as a draft.
max published this revision for review.May 25 2022, 2:03 PM
services/tunnelbroker/src/server.cpp
44 ↗(On Diff #13132)

Does comm::network::config::ConfigManager::getInstance().load() need to be repeated?

tomek requested changes to this revision.May 26 2022, 4:02 AM
tomek added 1 blocking reviewer(s): karol.
tomek added inline comments.
services/tunnelbroker/src/server.cpp
44 ↗(On Diff #13132)

It looks like we can conditionally choose the file path and the call load only once.

This revision now requires changes to proceed.May 26 2022, 4:02 AM
max marked 2 inline comments as done.
max added inline comments.
services/tunnelbroker/src/server.cpp
44 ↗(On Diff #13132)

Does comm::network::config::ConfigManager::getInstance().load() need to be repeated?

Yes, I think that makes sense here because the load(...) takes a different parameter here for the config file path according to the condition.

44 ↗(On Diff #13132)

It looks like we can conditionally choose the file path and the call load only once.

Yes, right!
I don't think putting out the comm::network::config::ConfigManager::getInstance() out of the condition and then calling load(...) on the instance makes sense here because we are using it only once.

karol added inline comments.
services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

I think Tomek meant something like this.

This revision now requires changes to proceed.May 26 2022, 6:05 AM
max marked 2 inline comments as done.
max added inline comments.
services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

I think Tomek meant something like this.

I think we have an agreement don't use ternary operators where it is possible. I've searched the service's codebase and we didn't use it.
cc @palys-swm

karol added inline comments.
services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

Ok, so we can do this

This revision now requires changes to proceed.May 26 2022, 6:52 AM
max marked 2 inline comments as done.
max added inline comments.
services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

Ok, so we can do this

It doesn't make sense to me. We are creating an additional variable here that in most cases will be used only once (if dev-mode is false) instead of just direct calling the method. In case of the dev-mode is true, we are rewriting this variable instead of just calling the method.
That case will look good if the code in the condition is complex, but we have only one method call in each occurrence.

I don't mind about this, but from my perspective, it looks like an overengineering here.

services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

It's really not a big deal, we can do whatever here. If you want more context on my perspective on ternary, you can see it here

(I'm not anti-ternary, I just think we should prioritize readability and not use ternary when it hurts readability)

This revision is now accepted and ready to land.May 27 2022, 2:23 AM
max added inline comments.
services/tunnelbroker/src/server.cpp
43–49 ↗(On Diff #13136)

It's really not a big deal, we can do whatever here. If you want more context on my perspective on ternary, you can see it here

(I'm not anti-ternary, I just think we should prioritize readability and not use ternary when it hurts readability)

Thanks for the comment @ashoat !

max marked an inline comment as done.

Rebased on master.