Use the server config instead of env variables for the API port and the server hostname #488

JonathanDCohen · 2019-09-13T20:52:53Z

No description provided.

src/server_manager/install_scripts/install_server.sh

src/shadowbox/README.md

src/server_manager/install_scripts/install_server.sh

src/shadowbox/docker/cmd.sh

src/shadowbox/docker/run_action.sh

src/shadowbox/server/main.ts

fortuna · 2019-09-14T20:24:00Z

src/shadowbox/docker/run_action.sh

+readonly STATE_DIR=$OUTLINE_DIR/persisted-state
+readonly STATE_CONFIG=$STATE_DIR/shadowbox_server_config.json
+mkdir -p $STATE_DIR && touch "$OUTLINE_DIR/config.json"
+[[ -e $STATE_CONFIG ]] || echo "{\"hostname\":\"127.0.0.1\", \"apiPort\":2357}" > $STATE_CONFIG
 source $ROOT_DIR/src/shadowbox/scripts/make_test_certificate.sh "${OUTLINE_DIR}"

 # TODO: mount a folder rather than individual files.
 declare -a docker_bindings=(
  -v "$OUTLINE_DIR/config.json":/root/shadowbox/shadowbox_config.json


Let's use shadowbox_config.json instead of config.json. It's less confusing.

In fact, this line is not needed if you are mounting the state dir. Both config files go there.

I don't understand why we can remove this line, the config isn't in the persisted-state directory.

See what we do in production:

outline-server/src/server_manager/install_scripts/install_server.sh

Lines 262 to 272 in 47446d4

declare -a docker_shadowbox_flags=(

--name shadowbox --restart=always --net=host

-v "${STATE_DIR}:${STATE_DIR}"

-e "SB_STATE_DIR=${STATE_DIR}"

-e "SB_PUBLIC_IP=${PUBLIC_HOSTNAME}"

-e "SB_API_PORT=${API_PORT}"

-e "SB_API_PREFIX=${SB_API_PREFIX}"

-e "SB_CERTIFICATE_FILE=${SB_CERTIFICATE_FILE}"

-e "SB_PRIVATE_KEY_FILE=${SB_PRIVATE_KEY_FILE}"

-e "SB_METRICS_URL=${SB_METRICS_URL:-}"

-e "SB_DEFAULT_SERVER_NAME=${SB_DEFAULT_SERVER_NAME:-}"

We actually mount the state dir. The config files are in it. Let's reproduce that, so it's closer to production and easier to understand. Using a different structure and file names for dev is confusing.

JonathanDCohen

Responded to comments. From offline discussion, also removed install script changes until after these changes get released. I messe dup Git royally though so definitely needs a close eye of review

src/server_manager/install_scripts/install_server.sh

src/shadowbox/README.md

src/shadowbox/docker/cmd.sh

JonathanDCohen · 2019-09-17T15:24:19Z

src/shadowbox/docker/run_action.sh

+readonly STATE_DIR=$OUTLINE_DIR/persisted-state
+readonly STATE_CONFIG=$STATE_DIR/shadowbox_server_config.json
+mkdir -p $STATE_DIR && touch "$OUTLINE_DIR/config.json"
+[[ -e $STATE_CONFIG ]] || echo "{\"hostname\":\"127.0.0.1\", \"apiPort\":2357}" > $STATE_CONFIG
 source $ROOT_DIR/src/shadowbox/scripts/make_test_certificate.sh "${OUTLINE_DIR}"

 # TODO: mount a folder rather than individual files.
 declare -a docker_bindings=(
  -v "$OUTLINE_DIR/config.json":/root/shadowbox/shadowbox_config.json


I don't understand why we can remove this line, the config isn't in the persisted-state directory.

src/shadowbox/docker/run_action.sh

JonathanDCohen · 2019-09-17T15:31:36Z

src/shadowbox/server/main.ts


  const DEFAULT_PORT = 8081;
-  const apiPortNumber = Number(process.env.SB_API_PORT || DEFAULT_PORT);
+  const apiPortNumber = Number(serverConfig.data().apiPort || process.env.SB_API_PORT || DEFAULT_PORT);


Done. Also edited the error message to show the user where the config is located so it's easier for them to edit the config.

src/shadowbox/server/main.ts

src/shadowbox/server/server_config.ts

JonathanDCohen · 2019-09-30T17:53:40Z

Friendly ping :)

fortuna

Looks good. One minor comment below.
I'm sorry for the delay!

fortuna · 2019-10-15T15:38:36Z

src/shadowbox/docker/run_action.sh

+readonly STATE_DIR=$OUTLINE_DIR/persisted-state
+readonly STATE_CONFIG=$STATE_DIR/shadowbox_server_config.json
+mkdir -p $STATE_DIR && touch "$OUTLINE_DIR/config.json"
+[[ -e $STATE_CONFIG ]] || echo "{\"hostname\":\"127.0.0.1\", \"apiPort\":2357}" > $STATE_CONFIG
 source $ROOT_DIR/src/shadowbox/scripts/make_test_certificate.sh "${OUTLINE_DIR}"

 # TODO: mount a folder rather than individual files.
 declare -a docker_bindings=(
  -v "$OUTLINE_DIR/config.json":/root/shadowbox/shadowbox_config.json


See what we do in production:

outline-server/src/server_manager/install_scripts/install_server.sh

Lines 262 to 272 in 47446d4

declare -a docker_shadowbox_flags=(

--name shadowbox --restart=always --net=host

-v "${STATE_DIR}:${STATE_DIR}"

-e "SB_STATE_DIR=${STATE_DIR}"

-e "SB_PUBLIC_IP=${PUBLIC_HOSTNAME}"

-e "SB_API_PORT=${API_PORT}"

-e "SB_API_PREFIX=${SB_API_PREFIX}"

-e "SB_CERTIFICATE_FILE=${SB_CERTIFICATE_FILE}"

-e "SB_PRIVATE_KEY_FILE=${SB_PRIVATE_KEY_FILE}"

-e "SB_METRICS_URL=${SB_METRICS_URL:-}"

-e "SB_DEFAULT_SERVER_NAME=${SB_DEFAULT_SERVER_NAME:-}"

We actually mount the state dir. The config files are in it. Let's reproduce that, so it's closer to production and easier to understand. Using a different structure and file names for dev is confusing.

src/shadowbox/server/main.ts

JonathanDCohen

Please double check carefully. I had to re do a lot of steps here as my git history got screwed up at some point.

src/shadowbox/integration_test/test.sh

src/shadowbox/docker/run_action.sh

src/shadowbox/integration_test/test.sh

At some point down the line I royally screwed up my git history and reverted a bunch of changes. I'm not sure what or when, so I `git reset --hard 1cd28e4` and tried to redo my past responses to comments from there.

JonathanDCohen requested a review from fortuna September 13, 2019 20:52

fortuna requested changes Sep 14, 2019

View reviewed changes

JonathanDCohen commented Sep 19, 2019

View reviewed changes

fortuna reviewed Sep 20, 2019

View reviewed changes

src/shadowbox/server/server_config.ts Outdated Show resolved Hide resolved

JonathanDCohen requested review from fortuna and alalamav October 7, 2019 20:54

fortuna reviewed Oct 15, 2019

View reviewed changes

JonathanDCohen force-pushed the cohenjon-use-config branch from 715f288 to 1cd28e4 Compare October 18, 2019 20:48

JonathanDCohen requested a review from fortuna October 18, 2019 20:51

JonathanDCohen commented Oct 18, 2019

View reviewed changes

fortuna reviewed Oct 21, 2019

View reviewed changes

src/shadowbox/integration_test/test.sh Show resolved Hide resolved

src/shadowbox/docker/run_action.sh Outdated Show resolved Hide resolved

JonathanDCohen requested a review from fortuna October 21, 2019 23:25

fortuna reviewed Oct 22, 2019

View reviewed changes

src/shadowbox/docker/run_action.sh Outdated Show resolved Hide resolved

src/shadowbox/integration_test/test.sh Outdated Show resolved Hide resolved

JonathanDCohen requested a review from fortuna October 22, 2019 17:04

fortuna approved these changes Oct 23, 2019

View reviewed changes

Jon Cohen added 6 commits November 18, 2019 17:22

Get public hostname from config file

8af4e39

Tidy up

fef60e3

Respond to Review Comments

3dfb817

At some point down the line I royally screwed up my git history and reverted a bunch of changes. I'm not sure what or when, so I `git reset --hard 1cd28e4` and tried to redo my past responses to comments from there.

Export SB_PUBLIC_IP in cmd.sh

45d4a77

respond to review comments

287da30

Review response

2a61138

JonathanDCohen force-pushed the cohenjon-use-config branch from 8c2f8c3 to 2a61138 Compare November 18, 2019 22:59

JonathanDCohen merged commit 46b7ee0 into master Nov 18, 2019

sbruens deleted the cohenjon-use-config branch March 5, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the server config instead of env variables for the API port and the server hostname #488

Use the server config instead of env variables for the API port and the server hostname #488

JonathanDCohen commented Sep 13, 2019

fortuna Sep 14, 2019

fortuna Sep 14, 2019

JonathanDCohen Sep 17, 2019

fortuna Oct 15, 2019

JonathanDCohen left a comment

JonathanDCohen Sep 17, 2019

JonathanDCohen Sep 17, 2019

JonathanDCohen commented Sep 30, 2019

fortuna left a comment

fortuna Oct 15, 2019

JonathanDCohen left a comment

	declare -a docker_shadowbox_flags=(
	--name shadowbox --restart=always --net=host
	-v "${STATE_DIR}:${STATE_DIR}"
	-e "SB_STATE_DIR=${STATE_DIR}"
	-e "SB_PUBLIC_IP=${PUBLIC_HOSTNAME}"
	-e "SB_API_PORT=${API_PORT}"
	-e "SB_API_PREFIX=${SB_API_PREFIX}"
	-e "SB_CERTIFICATE_FILE=${SB_CERTIFICATE_FILE}"
	-e "SB_PRIVATE_KEY_FILE=${SB_PRIVATE_KEY_FILE}"
	-e "SB_METRICS_URL=${SB_METRICS_URL:-}"
	-e "SB_DEFAULT_SERVER_NAME=${SB_DEFAULT_SERVER_NAME:-}"

Use the server config instead of env variables for the API port and the server hostname #488

Use the server config instead of env variables for the API port and the server hostname #488

Conversation

JonathanDCohen commented Sep 13, 2019

fortuna Sep 14, 2019

Choose a reason for hiding this comment

fortuna Sep 14, 2019

Choose a reason for hiding this comment

JonathanDCohen Sep 17, 2019

Choose a reason for hiding this comment

fortuna Oct 15, 2019

Choose a reason for hiding this comment

JonathanDCohen left a comment

Choose a reason for hiding this comment

JonathanDCohen Sep 17, 2019

Choose a reason for hiding this comment

JonathanDCohen Sep 17, 2019

Choose a reason for hiding this comment

JonathanDCohen commented Sep 30, 2019

fortuna left a comment

Choose a reason for hiding this comment

fortuna Oct 15, 2019

Choose a reason for hiding this comment

JonathanDCohen left a comment

Choose a reason for hiding this comment