Skip to content

Commit

Permalink
Logging retrieved db config values before using default values
Browse files Browse the repository at this point in the history
With reference to this commit:
#975 (comment)

So we have two key things to solve:
- Unnecessary variables like PATH, that may vary for each user to be removed (helps avoid "apple security" values).
- Need to see ENV vars values before being modified by default values in config.get() lines.

Two approaches:
1. Delete PATH, PYTHONPATH from the retrieved config.
- This wouldn't help much since we still need to handle the scenario when db config environment values haven't been set.

2. Using another config dictionary before modifying with default values
- If db config related keys exist, set it to the value from the retrieved config.
- Else if key does not exist in retrieved config, which means config file not present AND environment variables not set, then set it to None.
Note:
- This is not the same as changing the retrieved config value as the original dictionary is still intact.
- Additionally, in the case where config file not present AND environment variables not set, there are no values to before modifying with default values. So manually setting to None should be okay.
  • Loading branch information
Mahadik, Mukul Chandrakant authored and Mahadik, Mukul Chandrakant committed Aug 29, 2024
1 parent 0efbdc8 commit 9bba6c8
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
8 changes: 7 additions & 1 deletion emission/core/get_database.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,15 @@
config = ecbc.get_config('conf/storage/db.conf',
{"DB_HOST": "timeseries.url", "DB_RESULT_LIMIT": "timeseries.result_limit"})

db_config = {}
for key in ["DB_HOST", "DB_RESULT_LIMIT"]:
if key in config:
db_config[key] = config.get(key)
else:
db_config[key] = None
print("Retrieved config: %s" % db_config)
url = config.get("DB_HOST", "localhost")
result_limit = config.get("DB_RESULT_LIMIT", 250000)
print("Retrieved config: {'DB_HOST': '%s', 'DB_RESULT_LIMIT': '%s'}" % (url, result_limit))

try:
parsed=pymongo.uri_parser.parse_uri(url)
Expand Down
16 changes: 10 additions & 6 deletions emission/tests/storageTests/TestTokenQueries.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,19 +169,23 @@ def test_run_script_show(self):
esdt.insert({'token':'z'})
sp = subprocess.run(["python3", "bin/auth/insert_tokens.py", "--show"], capture_output=True)
# The first message is displayed when we run tests locally or run in the CI/CD, but with the local install
# The second is displayed when we run tests locally in a docker container or run in the docker CI; the `DB_HOST` is set to `db` by default
# The second is displayed when we run tests (the `DB_HOST` is set to `db` by default):
# a) in the docker CI or,
# b) locally in a docker container (ad-hoc testing environment; do not expect this to be used)
self.assertIn(sp.stdout,
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'localhost\', \'DB_RESULT_LIMIT\': \'250000\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'db\', \'DB_RESULT_LIMIT\': \'250000\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nx\ny\nz\n'
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': None, \'DB_RESULT_LIMIT\': None}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nx\ny\nz\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'db\', \'DB_RESULT_LIMIT\': None}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nx\ny\nz\n'
])

def test_run_script_empty(self):
sp = subprocess.run(["python3", "bin/auth/insert_tokens.py"], capture_output=True)
# The first message is displayed when we run tests locally or run in the CI/CD, but with the local install
# The second is displayed when we run tests locally in a docker container or run in the docker CI; the `DB_HOST` is set to `db` by default
# The second is displayed when we run tests (the `DB_HOST` is set to `db` by default):
# a) in the docker CI or,
# b) locally in a docker container (ad-hoc testing environment; do not expect this to be used)
self.assertIn(sp.stdout,
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'localhost\', \'DB_RESULT_LIMIT\': \'250000\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'db\', \'DB_RESULT_LIMIT\': \'250000\'}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nPlease provide the script with an argument. Use the "--help" option for more details\n'
[b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': None, \'DB_RESULT_LIMIT\': None}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL localhost\nPlease provide the script with an argument. Use the "--help" option for more details\n',
b'Config file not found, returning a copy of the environment variables instead...\nRetrieved config: {\'DB_HOST\': \'db\', \'DB_RESULT_LIMIT\': None}\nURL not formatted, defaulting to "Stage_database"\nConnecting to database URL db\nPlease provide the script with an argument. Use the "--help" option for more details\n'
])

#test that no two options can be used together
Expand Down

0 comments on commit 9bba6c8

Please sign in to comment.