-
Notifications
You must be signed in to change notification settings - Fork 1
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
NH-34752 Add config file tests + small changes #139
Conversation
}) | ||
|
||
# pylint: disable=unused-import | ||
from .fixtures.env_vars import fixture_mock_env_vars |
Check notice
Code scanning / CodeQL
Unused import
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.
Dismissing because pytest is using them.
from solarwinds_apm import apm_config | ||
|
||
# pylint: disable=unused-import | ||
from .fixtures.cnf_dict import fixture_cnf_dict |
Check notice
Code scanning / CodeQL
Unused import
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.
Dismissing because pytest is using them.
from .fixtures.cnf_file import ( | ||
fixture_cnf_file, | ||
fixture_cnf_file_invalid_json, | ||
) |
Check notice
Code scanning / CodeQL
Unused import
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.
Dismissing because pytest is using them.
fixture_cnf_file_invalid_json, | ||
) | ||
# pylint: disable=unused-import | ||
from .fixtures.env_vars import fixture_mock_env_vars |
Check notice
Code scanning / CodeQL
Unused import
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.
Dismissing because pytest is using them.
@@ -594,7 +596,7 @@ def update_with_env_var(self) -> None: | |||
available_envvs = set(self.__config.keys()) | |||
# TODO after alpha: is_lambda | |||
for key in available_envvs: | |||
if key in {"inst_enabled", "transaction", "inst"}: | |||
if key == "transaction": |
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.
Is there a reason for having transaction
in self.__config.keys()
. I think removing it from there will nullify this branch.
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.
It's there as part of one way to store transaction
with a default and transaction.prepend_domain_name
as a nested object if set by customer, but I'm not convinced this is a good reason!
Right now an accepted config file would be:
{
"transaction": {
"prependDomain": false
},
"transactionSettings": []
}
But it might make more sense to customer if it were:
{
"prependDomain": false,
"transactionSettings": []
}
I'll change this in a next PR.
This mainly adds config file support tests. Changes:
test_apm_config.py
to its own subdirget_cnf_dict
helper.test_apm_config_cnf_file.py
to test different scenarios for config file reading, updating config using file (with or without env vars also specified).This PR also includes some small code changes:
inst
andinst_enabled
that were copied from AO but won't be in SWOget_cnf_dict
helper out of existingupdate_with_cnf_file
for easier testingSince code was changed, here is a test trace that looks ok.
Please let me know if any suggestions! 😺