Page MenuHomePhabricator
Differential D4638

[services] Tunnelbroker - Changing the `ConfigManager` load function to include the sandbox condition
ClosedPublic
Actions

Authored by • max on Jul 26 2022, 4:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:38 PM
Unknown Object (File)
Tue, Nov 19, 4:35 PM
Unknown Object (File)
Thu, Nov 14, 10:27 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• karol
tomek

Details

Reviewers
tomek
• karol
Commits
rCOMM992cd07cc7a4: [services] Tunnelbroker - Changing the `ConfigManager` load function to include…
Summary

Changing the ConfigManager.load() function to include the sandbox config condition to avoid duplications and fix some of the tests.

Test Plan

Run build with tests: yarn run-tunnelbroker-service-in-sandbox or build inside the Docker container with the export COMM_TEST_SERVICES=1 command before the cmake command.

Diff Detail

Repository
rCOMM Comm
Branch
fix-log-init-in-tests
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Jul 26 2022, 4:06 AM
• max held this revision as a draft.
Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 26 2022, 4:07 AM
• max published this revision for review.Jul 26 2022, 4:10 AM
• max edited the summary of this revision. (Show Details)
• max added reviewers: tomek, • karol.
Harbormaster completed remote builds in B10873: Diff 14949.Jul 26 2022, 4:18 AM
tomek requested changes to this revision.EditedJul 26 2022, 5:55 AM

What do you think about introducing this change in load method of config manager? (to avoid the duplication)

This revision now requires changes to proceed.Jul 26 2022, 5:55 AM
• karol accepted this revision.Jul 26 2022, 5:59 AM

How did it work before without an argument provided?

• max requested review of this revision.Jul 26 2022, 6:17 AM
In D4638#133143, @tomek wrote:

What do you think about introducing this change in load method of config manager? (to avoid the duplication)

The main changes are already done (landed) in D4129. It's a follow-up fix! I've missed this and some of the tests failed. This diff is a fix.

tomek accepted this revision.Jul 26 2022, 6:26 AM
In D4638#133161, @max wrote:
In D4638#133143, @tomek wrote:

What do you think about introducing this change in load method of config manager? (to avoid the duplication)

The main changes are already done (landed) in D4129. It's a follow-up fix! I've missed this and some of the tests failed. This diff is a fix.

Ok, got it. After seeing this code I think it would be better to remove the parameter from load method and move the if there. Current solution requires adding this logic to every test case - this isn't maintainable. If the change from this diff was included in D4129 I would suggest the same. Can we do this?

This revision is now accepted and ready to land.Jul 26 2022, 6:26 AM
• max planned changes to this revision.Jul 26 2022, 7:19 AM
• max updated this revision to Diff 14980.Jul 26 2022, 7:28 AM

Changing the ConfigManager.load() function to include the sandbox config condition to avoid duplications and fix some of the tests.

This revision is now accepted and ready to land.Jul 26 2022, 7:28 AM
• max requested review of this revision.EditedJul 26 2022, 7:30 AM
• max retitled this revision from [services] Tunnelbroker - Fix tests build due to the ConfigManager changes to [services] Tunnelbroker - Changing the `ConfigManager` load function to include the sandbox condition.
• max edited the summary of this revision. (Show Details)
In D4638#133182, @tomek wrote:
In D4638#133161, @max wrote:
In D4638#133143, @tomek wrote:

What do you think about introducing this change in load method of config manager? (to avoid the duplication)

The main changes are already done (landed) in D4129. It's a follow-up fix! I've missed this and some of the tests failed. This diff is a fix.

Ok, got it. After seeing this code I think it would be better to remove the parameter from load method and move the if there. Current solution requires adding this logic to every test case - this isn't maintainable. If the change from this diff was included in D4129 I would suggest the same. Can we do this?

I've checked the function usage and yes we can do this instead. Good catch, thanks! The load() function was changed and the old function moved to the private space and renamed to loadConfigFile().

Harbormaster completed remote builds in B10893: Diff 14980.Jul 26 2022, 7:38 AM
• max updated this revision to Diff 15003.Jul 27 2022, 3:14 AM

Rebase on master changes.

Harbormaster completed remote builds in B10904: Diff 15003.Jul 27 2022, 3:27 AM
tomek accepted this revision.Jul 27 2022, 4:47 AM
tomek added inline comments.
services/tunnelbroker/src/server.cpp
43 ↗(On Diff #15003)

We should keep tunnelbroker because argv[0] can contain a lot of noise, e.g. path.

This revision is now accepted and ready to land.Jul 27 2022, 4:47 AM
• max updated this revision to Diff 15019.Jul 27 2022, 8:32 AM
• max marked an inline comment as done.

Using 'tunnelbroker' in InitLogging(...).

• max added inline comments.Jul 27 2022, 8:37 AM
services/tunnelbroker/src/server.cpp
43 ↗(On Diff #15003)

We should keep tunnelbroker because argv[0] can contain a lot of noise, e.g. path.

Agree with that. Thanks, @tomek.

This revision was landed with ongoing or failed builds.Jul 27 2022, 8:42 AM
Closed by commit rCOMM992cd07cc7a4: [services] Tunnelbroker - Changing the `ConfigManager` load function to include… (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMM992cd07cc7a4: [services] Tunnelbroker - Changing the `ConfigManager` load function to include….
Harbormaster completed remote builds in B10915: Diff 15019.Jul 27 2022, 8:45 AM

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
test/
8 lines
8 lines

Diff 14949

View Options

services/tunnelbroker/test/AmqpManagerTest.cpp

Loading...
View Options

services/tunnelbroker/test/DatabaseManagerTest.cpp

Loading...