-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redesign Build Deployment Process (External) #961
Changes from 59 commits
93c2232
a80d7e4
20e1856
4a1005e
d4dad4a
02c7fc7
4aeb4a6
2531672
ea57afa
e8344e9
7f0d5f0
05551c8
cabc98a
c39b537
385bc97
273c2ca
2fdb469
91abdea
bbdaaae
5d0ca02
2e8a911
f5aae47
051ef66
6c44865
4569dfb
b562c2c
25109b3
709f3cf
4b42185
12d09ae
94c129b
0dd1245
5434914
0fd3e9d
ab399ea
91c0c1f
acecf3e
e778b3f
a0190d4
776f0b9
17ac3cc
706f74c
d724fd1
e6a2d79
4b3dfdf
f033f06
1207d79
f306e8c
00d9565
f182790
f1869ab
8f05955
912bd34
738b629
7102508
3d77439
a0f2424
9fb0e5a
18a8872
53015c7
41a410c
7405ff1
cd62247
ebc8188
41ae79f
290b0fc
29869cd
38209ef
da485c2
e6b388b
bb03c42
cf50d17
6a21f5d
b961417
a245e7d
2067055
3000758
6a8a13f
fc183cb
51e16de
7ab37b6
727c00c
7f1be92
7177e71
168ef10
3dea305
a0f0c6a
10624a6
0fe9476
11d2a89
4cc4c58
357d4b8
b6f59b0
ec38835
1a0d451
2fe0816
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -7,22 +7,11 @@ | |||
echo ${DB_HOST} | ||||
if [ -z ${DB_HOST} ] ; then | ||||
local_host=`hostname -i` | ||||
jq --arg db_host "$local_host" '.timeseries.url = $db_host' conf/storage/db.conf.sample > conf/storage/db.conf | ||||
else | ||||
jq --arg db_host "$DB_HOST" '.timeseries.url = $db_host' conf/storage/db.conf.sample > conf/storage/db.conf | ||||
export DB_HOST=$local_host | ||||
echo "Setting db host environment variable to localhost" | ||||
fi | ||||
cat conf/storage/db.conf | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have removed this, there is really no reason to |
||||
#set Web Server host using environment variable | ||||
echo ${WEB_SERVER_HOST} | ||||
if [ -z ${WEB_SERVER_HOST} ] ; then | ||||
local_host=`hostname -i` | ||||
sed "s_localhost_${local_host}_" conf/net/api/webserver.conf.sample > conf/net/api/webserver.conf | ||||
else | ||||
sed "s_localhost_${WEB_SERVER_HOST}_" conf/net/api/webserver.conf.sample > conf/net/api/webserver.conf | ||||
fi | ||||
cat conf/net/api/webserver.conf | ||||
|
||||
if [ -z ${LIVERELOAD_SRC} ] ; then | ||||
echo "Live reload disabled, " | ||||
else | ||||
|
@@ -35,6 +24,7 @@ fi | |||
|
||||
#TODO: start cron jobs | ||||
# change python environment | ||||
echo "Starting up e-mission-environment..." | ||||
source setup/activate.sh | ||||
|
||||
# launch the webapp | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,21 @@ | ||
# This is a basic workflow to help you get started with Actions | ||
|
||
name: docker image | ||
|
||
# Controls when the action will run. Triggers the workflow on push or pull request | ||
# events but only for the master branch | ||
on: | ||
push: | ||
branches: [ master, gis-based-mode-detection ] | ||
|
||
branches: [ master, gis-based-mode-detection, consolidate-differences ] | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nataliejschultz why do we have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has not been fixed yet. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to leave it and continue to test in GH actions as the cleanup changes are made, but I removed it now! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has still not been fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shankari I have noticed a strange issue that I described to @MukuFlash03 ; some of the changes I made (such as removing consolidate-differences branch) do not appear to have taken effect in the diff display here. However, when I go to "edit file" in GitHub, the file is correct. It also looks correct in my IDE. I'm not sure why it's doing this 😞 |
||
# Env variable | ||
env: | ||
DOCKER_USER: ${{secrets.DOCKER_USER}} | ||
DOCKER_PASSWORD: ${{secrets.DOCKER_PASSWORD}} | ||
|
||
# A workflow run is made up of one or more jobs that can run sequentially or in parallel | ||
jobs: | ||
# This workflow contains a single job called "build" | ||
build: | ||
# The type of runner that the job will run on | ||
runs-on: ubuntu-latest | ||
|
||
# Steps represent a sequence of tasks that will be executed as part of the job | ||
outputs: | ||
date: ${{ steps.date.outputs.date }} | ||
|
||
steps: | ||
# Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it | ||
- uses: actions/checkout@v2 | ||
- name: docker login | ||
run: | # log into docker hub account | ||
|
@@ -46,3 +38,40 @@ jobs: | |
- name: push docker image | ||
run: | | ||
docker push $DOCKER_USER/${GITHUB_REPOSITORY#*/}:${GITHUB_REF##*/}_${{ steps.date.outputs.date }} | ||
|
||
- name: Create a text file | ||
run: | | ||
echo ${{ steps.date.outputs.date }} > tag_file.txt | ||
echo "Created tag text file" | ||
|
||
- name: Upload Artifact | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: docker-image-tag | ||
path: tag_file.txt | ||
overwrite: true | ||
|
||
Comment on lines
+48
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See Mukul's comment in the issue:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the issue here. You have to deal with dispatch and push separately anyway - one of them will have the image tag and the other will not. And given that we have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do see now why we do not need the artifact for the push event. Detailed comments made here in public-dash Redesign PR. The comments talk about the corresponding artifact download in the dashboard repos and the explanation is applicable to the upload artifact on the server side as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MukuFlash03 I utilize the artifacts to get the tags to the internal repo, so please don't remove them yet!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nataliejschultz For now, I've tested removing only the Can you please confirm if in this case it is alright to remove the artifact dealing with Push event from the dashboard workflows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MukuFlash03 sorry, I misunderstood what you were changing. It is okay with me to remove the |
||
dispatch: | ||
needs: build | ||
runs-on: ubuntu-latest | ||
|
||
env: | ||
DOCKER_IMAGE_TAG: ${{ needs.build.outputs.date }} | ||
|
||
strategy: | ||
matrix: | ||
repo: ['e-mission/op-admin-dashboard', 'e-mission/em-public-dashboard'] | ||
|
||
steps: | ||
- uses: actions/checkout@v4 | ||
|
||
- name: Trigger workflow in admin-dash, public-dash | ||
# TODO: Create Fine-grained token with "Actions: write" permissions | ||
run: | | ||
shankari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
curl -L \ | ||
-X POST \ | ||
-H "Accept: application/vnd.github+json" \ | ||
-H "Authorization: Bearer ${{ secrets.GH_FG_PAT_TAGS }}" \ | ||
-H "X-GitHub-Api-Version: 2022-11-28" \ | ||
https://api.github.com/repos/${{ matrix.repo }}/actions/workflows/image_build_push.yml/dispatches \ | ||
-d '{"ref":"master", "inputs": {"docker_image_tag" : "${{ env.DOCKER_IMAGE_TAG }}"}}' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ RUN bash -c "./.docker/setup_config.sh" | |
|
||
# #declare environment variables | ||
ENV DB_HOST='' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For example, you could also set the default value of |
||
ENV WEB_SERVER_HOST='' | ||
ENV WEB_SERVER_HOST=0.0.0.0 | ||
|
||
ENV LIVERELOAD_SRC='' | ||
ENV STUDY_CONFIG='' | ||
|
shankari marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"intake.segmentation.section_segmentation.sectionValidityAssertions": true, | ||
"intake.cleaning.clean_and_resample.speedDistanceAssertions": false, | ||
"intake.cleaning.clean_and_resample.sectionValidityAssertions": false, | ||
"intake.cleaning.filter_accuracy.enable": false, | ||
"classification.inference.mode.useAdvancedFeatureIndices": true, | ||
"classification.inference.mode.useBusTrainFeatureIndices": true, | ||
"classification.validityAssertions": true, | ||
"output.conversion.validityAssertions": true, | ||
"section.startStopRadius": 150, | ||
"section.endStopRadius": 150, | ||
"analysis.result.section.key": "analysis/inferred_section", | ||
"userinput.keylist": ["manual/mode_confirm", "manual/purpose_confirm", "manual/replaced_mode", "manual/trip_user_input"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,17 @@ | ||
import json | ||
import os | ||
|
||
def get_config_data(): | ||
try: | ||
print("Trying to open debug.conf.json") | ||
config_file = open('conf/analysis/debug.conf.json') | ||
Comment on lines
+6
to
7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not fully convinced that this is the right approach. If we keep this backwards compat code around forever, we don't have any motivation for people to change to the new structure in the future. Having said that, I will not hold up the merge for this, but I do want to make sure that we have a plan to remove this in a year or so, and to notify users to change their config files, so that we don't end up with a bunch of hacky backwards compat code strewn all around the codebase. |
||
except: | ||
print("analysis.debug.conf.json not configured, falling back to sample, default configuration") | ||
config_file = open('conf/analysis/debug.conf.json.sample') | ||
if os.getenv("PROD_STAGE") == "TRUE": | ||
print("In production environment, opening internal debug.conf") | ||
config_file = open('conf/analysis/debug.conf.internal.json') | ||
else: | ||
print("analysis.debug.conf.json not configured, falling back to sample, default configuration") | ||
config_file = open('conf/analysis/debug.conf.json.sample') | ||
shankari marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ret_val = json.load(config_file) | ||
config_file.close() | ||
return ret_val | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import json | ||
import logging | ||
import os | ||
|
||
def get_config_data_from_env(): | ||
config_data_env = { | ||
"timeseries": { | ||
"url": os.getenv('DB_HOST', "localhost"), | ||
"result_limit": os.getenv('DB_TS_RESULT_LIMIT', 250000) | ||
} | ||
} | ||
return config_data_env | ||
|
||
def get_config_data(): | ||
try: | ||
config_file = open('conf/storage/db.conf') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also don't think we need this given that we are going to use the environment variable going forward and it falls back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also remove the sample file to avoid confusion since we don't use it. Note that before this change, we used to fall back to the sample file if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Our logic here was that we wanted to maintain the functionality of db.conf for users running the server locally, in case they have it set in a certain way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would be fine with retaining this for now as a backwards compat hack but it should be removed in the next release. |
||
ret_val = json.load(config_file) | ||
config_file.close() | ||
except: | ||
ret_val = get_config_data_from_env() | ||
if ret_val["timeseries"]["url"] == "localhost": | ||
print("storage not configured, falling back to sample, default configuration") | ||
return ret_val | ||
|
||
config_data = get_config_data() | ||
|
||
def get_config(): | ||
return config_data | ||
|
||
def reload_config(): | ||
global config_data | ||
config_data = get_config_data() |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,9 +6,8 @@ cd /src/e-mission-server | |
echo ${DB_HOST} | ||
if [ -z ${DB_HOST} ] ; then | ||
local_host=`hostname -i` | ||
sed "s_localhost_${local_host}_" conf/storage/db.conf.sample > conf/storage/db.conf | ||
else | ||
sed "s_localhost_${DB_HOST}_" conf/storage/db.conf.sample > conf/storage/db.conf | ||
export DB_HOST=$local_host | ||
echo "Setting db host environment variable to localhost" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when fixing the dockerized start script, we should fix this as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this environment variable set here anyway, since start_integration_tests.sh runs via a Dockerfile whose corresponding compose has the variable hardcoded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nataliejschultz fair, but the integration tests are not the only way that people run the server. Developers run the server on their laptops to make changes as well. You need to now change the README to indicate that the environment variable should be set ... |
||
fi | ||
cat conf/storage/db.conf | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,27 +51,17 @@ | |
import emission.storage.timeseries.cache_series as esdc | ||
import emission.core.timer as ect | ||
import emission.core.get_database as edb | ||
import emission.net.api.config as enac | ||
|
||
try: | ||
config_file = open('conf/net/api/webserver.conf') | ||
except: | ||
logging.debug("webserver not configured, falling back to sample, default configuration") | ||
config_file = open('conf/net/api/webserver.conf.sample') | ||
|
||
OPENPATH_URL="https://www.nrel.gov/transportation/openpath.html" | ||
STUDY_CONFIG = os.getenv('STUDY_CONFIG', "stage-program") | ||
|
||
config_data = json.load(config_file) | ||
config_file.close() | ||
static_path = config_data["paths"]["static_path"] | ||
python_path = config_data["paths"]["python_path"] | ||
server_host = config_data["server"]["host"] | ||
server_port = config_data["server"]["port"] | ||
socket_timeout = config_data["server"]["timeout"] | ||
log_base_dir = config_data["paths"]["log_base_dir"] | ||
auth_method = config_data["server"]["auth"] | ||
aggregate_call_auth = config_data["server"]["aggregate_call_auth"] | ||
not_found_redirect = config_data["paths"].get("404_redirect", OPENPATH_URL) | ||
enac.reload_config() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we reload the config only here and nowhere else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The STUDY_CONFIG environment variable is pulled directly above, so changing this value and reloading allows testers/users to set the variable to different configs in the container using export and test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This statement doesn't answer my question. I asked why the config is reloaded here and not anywhere else.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nataliejschultz the screenshot is not loading for me (see screenshot below). We weren't re-loading the config before and the tests passed. I really don't see any reason why the config needs to be reloaded. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@shankari here is the screenshot
|
||
static_path = enac.get_config()["static_path"] | ||
server_host = enac.get_config()["server_host"] | ||
server_port = enac.get_config()["server_port"] | ||
socket_timeout = enac.get_config()["socket_timeout"] | ||
auth_method = enac.get_config()["auth_method"] | ||
aggregate_call_auth = enac.get_config()["aggregate_call_auth"] | ||
not_found_redirect = enac.get_config()["not_found_redirect"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this ever tested? I don't think that it would have ever worked.
And we didn't modify the path before returning it
|
||
|
||
BaseRequest.MEMFILE_MAX = 1024 * 1024 * 1024 # Allow the request size to be 1G | ||
# to accomodate large section sizes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import json | ||
import logging | ||
import os | ||
|
||
def get_config_data_from_env(): | ||
config_data_env = { | ||
"static_path": os.getenv('WEB_SERVER_STATIC_PATH', "webapp/www/"), | ||
"server_host": os.getenv('WEB_SERVER_HOST', "0.0.0.0"), | ||
"server_port": os.getenv('WEB_SERVER_PORT', "8080"), | ||
"socket_timeout": os.getenv('WEB_SERVER_TIMEOUT', "3600"), | ||
"auth_method": os.getenv('WEB_SERVER_AUTH', "skip"), | ||
"aggregate_call_auth": os.getenv('WEB_SERVER_AGGREGATE_CALL_AUTH', "no_auth"), | ||
"not_found_redirect": os.getenv('WEB_SERVER_REDIRECT_URL', "https://www.nrel.gov/transportation/openpath.html") | ||
} | ||
return config_data_env | ||
|
||
|
||
def get_config_data(): | ||
try: | ||
config_file = open('conf/net/api/webserver.conf') | ||
ret_val = json.load(config_file) | ||
config_file.close() | ||
except: | ||
# if check_unset_env_vars(): | ||
logging.debug("webserver not configured, falling back to sample, default configuration") | ||
ret_val = get_config_data_from_env() | ||
return ret_val | ||
|
||
config_data = get_config_data() | ||
|
||
def get_config(): | ||
return config_data | ||
|
||
def reload_config(): | ||
global config_data | ||
config_data = get_config_data() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import json | ||
import logging | ||
import os | ||
|
||
def get_config_data_from_env(): | ||
config_data_env = { | ||
"provider": os.getenv("PUSH_PROVIDER"), | ||
"server_auth_token": os.getenv("PUSH_SERVER_AUTH_TOKEN"), | ||
"app_package_name": os.getenv("PUSH_APP_PACKAGE_NAME"), | ||
"ios_token_format": os.getenv("PUSH_IOS_TOKEN_FORMAT") | ||
} | ||
return config_data_env | ||
|
||
def get_config_data(): | ||
try: | ||
config_file = open('conf/net/ext_service/push.json') | ||
ret_val = json.load(config_file) | ||
config_file.close() | ||
except: | ||
logging.warning("net.ext_service.push.json not configured, checking environment variables...") | ||
ret_val = get_config_data_from_env() | ||
# Check if all PUSH environment variables are not set | ||
if (not any(ret_val.values())): | ||
raise TypeError | ||
return ret_val | ||
|
||
try: | ||
config_data = get_config_data() | ||
except: | ||
logging.warning("All push environment variables are set to None") | ||
|
||
def get_config(): | ||
return config_data | ||
|
||
def reload_config(): | ||
global config_data | ||
config_data = get_config_data() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. @nataliejschultz have you tried testing this with
DB_HOST
not set? I bet right now that it doesn't workThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shankari You're right, it does not work without DB_HOST set. I ran the server using my locally built image:
and before I could exec into the container, it gave me this error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shankari I've tried several different ways to export the DB_HOST environment variable, but kept getting the
connection refused
error. I decided to try running the server container as it currently is in production by building a local image with an added echo.The echo I added is in docker_start_script.sh, after it's supposedly set with the sample file:
Container log:
I got the connection refused error again. Thoughts:
My db container is the problemGoing to try a new db container first and see if that's the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried new DB container:
docker run --name db -d -p 27017:27017 --network emission mongo:4.4.0
Ran the exact same image as before, and got the same connection refused error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the challenge outlined in
#961 (comment)
The previous codebase assumed that if the DB_HOST was not set, the DB and the webapp were running on the same host. Obviously, if you try to test it with containers, it won't work.
However, with the new codebase, even if you run the DB and the webapp on the same container, it won't work because of #961 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the group meeting with @shankari, we clarified that:
else
was removed, so this code should work as expectedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this doesn't work with containers, then do we even need this functionality in a file that only runs when the dockerfile is run?? It makes sense in start_script.sh, but maybe not here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was hoping you would come to that. A lot of these decisions were made a long time ago, when our deployment profile was very different than what we do now. If you recall, part of this redesign was to simplify and unify these startup scripts.
You should ask yourself these questions, answer them and then keep only the parts that are relevant.
Even in
start_script.sh
, why do you need this, given that there is a fallback in the config file?And if you do need it, why does it need to be
hostname -i
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I finally understand.
DB_HOST will never need to be set when running with containers
start_script.sh runs all the tests, and the tests (and every server call in general) call get_database.py which gets the environment variable from emission.core.config. This has the fallback:
using hostname -i or localhost makes sense when not using containers, and the fallback to localhost should account for that. It seems like using localhost would be a rare case for someone running the server locally without containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nataliejschultz it is actually the most common case (at least now) for developers who are writing server code, aka making changes to server functionality and features, and not only the build scripts