Paths

Table of Contentst

[services] Tunnelbroker - Moving the config file to the `~/.config` directory and removing the sandbox config
ClosedPublic
Actions

Authored by • max on Dec 1 2022, 8:35 AM.

Details

Reviewers

• jon
varun
bartek
tomek
ashoat

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rCOMM824a62323ca2: [services] Tunnelbroker - Moving the config file to the `~/.config` directory…

Summary

Moving the config file for Tunnelbroker to the ~/.config directory to unify services configuration. Removing using the "sandbox" config file. Updating services documentation about the new path and removing the sandbox config.

Linear task and full context: ENG-1680

Test Plan

Tunnelbroker service is successfully built.
Config file is successfully read from the new path.

Diff Detail

Repository

rCOMM Comm

Branch

move-config-file

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Dec 1 2022, 8:35 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptDec 1 2022, 8:35 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B13918: Diff 19073.Dec 1 2022, 8:51 AM

• max published this revision for review.Dec 1 2022, 11:48 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: varun, bartek. • max added 1 blocking reviewer(s): • jon.

Should also remove the corresponding comm::network::tools::isSandbox() method if it's going to exist within the config file.

This revision is now accepted and ready to land.Dec 2 2022, 9:15 AM

AFAIK the comm::network::tools::isSandbox() is still used in a few other places

In D5790#173107, @jon wrote:

Should also remove the corresponding comm::network::tools::isSandbox() method if it's going to exist within the config file.

It's more complex and used in other C++ services. The task of removing isSandbox is tracked by the ENG-2296.

• max added a reviewer: tomek.Dec 2 2022, 10:11 AM

This revision now requires review to proceed.Dec 2 2022, 10:11 AM

Adding @ashoat because the documentation is changed.

docs/dev_services.md
42–43	Is it a good practice to use this directory? Is it possible that we will have some conflicts with other tools which might decide to use the same directory name?
59	This doesn't read well. Is this sentence inverted in some way?
81	Is it a good idea to just delete this part? Is it no longer relevant, or should be just updated?

This revision now requires changes to proceed.Dec 5 2022, 4:42 AM

ashoat added a reviewer: Restricted Owners Package.Dec 5 2022, 6:23 AM

ashoat added inline comments.

docs/dev_services.md
59	Yeah this needs to be fixed up

ashoat requested changes to this revision.Dec 5 2022, 6:23 AM

• max planned changes to this revision.Dec 5 2022, 8:56 AM

• max marked an inline comment as done.

Fix configuration file template sentence in a doc.

docs/dev_services.md
42–43	Is it a good practice to use this directory? Is it possible that we will have some conflicts with other tools which might decide to use the same directory name? We have a discussion with @jon about the flexibility of services configuration and `~/.config/$service.ini` seems a good way to go on. Not seeing any conflicts here.
59	This doesn't read well. Is this sentence inverted in some way? Hope it reads better now.
81	Is it a good idea to just delete this part? Is it no longer relevant, or should be just updated? We didn't remove the sandbox yet. We've removed usage of the `tunnelbroker-dev` config file and use a single config file. That's why I've removed only the part.

Harbormaster completed remote builds in B13998: Diff 19171.Dec 6 2022, 5:31 AM

Is it possible for anybody else to rewrite @max's English before I take a pass? Please re-add me once it's ready to go

In D5790#173953, @ashoat wrote:

Is it possible for anybody else to rewrite @max's English before I take a pass? Please re-add me once it's ready to go

I think @varun can take a quick look.

tomek accepted this revision.Dec 6 2022, 9:08 AM

tomek added 1 blocking reviewer(s): varun.

varun requested changes to this revision.Dec 6 2022, 7:27 PM

varun added inline comments.

docs/dev_services.md
59 ↗	(On Diff #19171)	Did you mean `services/tunnelbroker/tunnelbroker-sandbox.ini`? Also I don't think we need to mention this at all since the user is probably just going to copy-paste the above config, right?

This revision now requires changes to proceed.Dec 6 2022, 7:27 PM

Removing a sentence with the config file.

• max marked an inline comment as done.Dec 7 2022, 3:14 AM

• max added inline comments.

docs/dev_services.md
59 ↗	(On Diff #19171)	Did you mean `services/tunnelbroker/tunnelbroker-sandbox.ini`? Yes. `tunnelbroker-sandbox.ini` moved to `tunnelbroker.ini`. Also I don't think we need to mention this at all since the user is probably just going to copy-paste the above config, right? I think you're right. We can just remove it as the config will be copy pasted from the doc or created from the scratch for production. I've removed it.