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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

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

How did it work before without an argument provided?

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.

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

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

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

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.