Skip to content
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

Add configuration variable expansion #681

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

jhakonen
Copy link
Contributor

This PR implements issue #560.

I went with the variable pattern of ${ENV:NAME} or $ENV:NAME as suggested by jpmens. Including also support for loading file content with ${FILE:file/path}. I used FILE instead of FS as I think it is more explicit.

I didn't add support for HashiCorp's Vault as I don't have personal experience of using it, but provided that there's a way to fetch vault's value then it is easy to add support for it by modifying VariableInterpolation.sources.

Copy link
Collaborator

@jpmens jpmens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, and thank you for your contribution!

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.45 🎉

Comparison is base (29ea2f5) 48.88% compared to head (b5d3b3b) 49.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
+ Coverage   48.88%   49.33%   +0.45%     
==========================================
  Files          81       81              
  Lines        3938     3973      +35     
==========================================
+ Hits         1925     1960      +35     
  Misses       2013     2013              
Flag Coverage Δ
unittests 49.33% <100.00%> (+0.45%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mqttwarn/configuration.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@amotl
Copy link
Member

amotl commented Jul 24, 2023

Thank you very much, Janne! 💯

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear Janne,

thanks for your excellent patch. I've just added a question about the test suite.

With kind regards,
Andreas.

Comment on lines +129 to +143
sources = {
"SRC_1": create_source(
{
"PASSWORD_1": "my-password",
"PASSWORD_2": "super-secret",
"PÄSSWÖRD_3": "non-ascii-secret",
"/path/to/password.txt": "file-contents",
}
),
"SRC_2": create_source(
{
"PASSWORD_1": "p4ssw0rd",
}
),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SRC_1 and SRC_2 are two synthetic variable interpolation types like ENV or FILE, but only used within the test suite?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'm testing expand_vars() function here directly, bypassing concrete implementation of ENV and FILE sources, so I've replaced them with these fakes.

Comment on lines +166 to +167
with pytest.raises(KeyError, match=re.escape("$SRC_1:VARIABLE: 'Variable not found'")):
mqttwarn.configuration.expand_vars("-->$SRC_1:VARIABLE<--", {"SRC_1": empty_source})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my statement above is true, we may have a leak in the suite between test functions. I.e., the types SRC_1 and SRC_2 are registered within one test function, but used within another. a) What if those would run in a different order? b) Will it still work when exclusively running this function like pytest -k test_expand_vars_variable_not_found?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test functions shouldn't affect each other as there is no global state used in expand_vars(). It doesn't have any side-effect when called.

Here in this test function we're passing in a new dict of sources {"SRC_1": empty_source} which is not same dict as the sources in test_expand_vars_ok() test function.

I tried moving test_expand_vars_variable_not_found() before test_expand_vars_ok() but it passes still fine:

[nix-shell:~/mqttwarn]$ pytest --no-header --disable-warnings --no-cov -k test_expand_vars
=================================================== test session starts ===================================================
collected 318 items / 304 deselected / 1 skipped / 14 selected                                                            

tests/test_configuration.py::test_expand_vars_variable_not_found PASSED                                             [  7%]
tests/test_configuration.py::test_expand_vars_ok[my-password-my-password] PASSED                                    [ 14%]
tests/test_configuration.py::test_expand_vars_ok[$SRC_1:PASSWORD_1-my-password] PASSED                              [ 21%]
tests/test_configuration.py::test_expand_vars_ok[$SRC_1:PASSWORD_2-super-secret] PASSED                             [ 28%]
tests/test_configuration.py::test_expand_vars_ok[-->$SRC_1:PASSWORD_1<----->my-password<--] PASSED                  [ 35%]
tests/test_configuration.py::test_expand_vars_ok[$SRC_2:PASSWORD_1-p4ssw0rd] PASSED                                 [ 42%]
tests/test_configuration.py::test_expand_vars_ok[$SRC_1:P\xc4SSW\xd6RD_3-non-ascii-secret] PASSED                   [ 50%]
tests/test_configuration.py::test_expand_vars_ok[${SRC_1:PASSWORD_1}-my-password] PASSED                            [ 57%]
tests/test_configuration.py::test_expand_vars_ok[${SRC_1:/path/to/password.txt}-file-contents] PASSED               [ 64%]
tests/test_configuration.py::test_expand_vars_ok[${SRC_1:PASSWORD_1} ${SRC_1:PASSWORD_2}-my-password super-secret] PASSED [ 71%]
tests/test_configuration.py::test_expand_vars_ok[$SRC_1:PASSWORD_1 ${SRC_1:/path/to/password.txt} $SRC_1:PASSWORD_1-my-password file-contents my-password] PASSED [ 78%]
tests/test_configuration.py::test_expand_vars_ok[${SRC_1:/path/to/password.txt} $SRC_1:PASSWORD_1 ${SRC_1:/path/to/password.txt}-file-contents my-password file-contents] PASSED [ 85%]
tests/test_configuration.py::test_expand_vars_ok[/$SRC_1:PASSWORD_1/$SRC_1:PASSWORD_2/foo.txt-/my-password/super-secret/foo.txt] PASSED [ 92%]
tests/test_configuration.py::test_expand_vars_variable_type_not_supported PASSED                                    [100%]

================================================= short test summary info =================================================
SKIPPED [1] tests/services/test_apns.py:11: The `apns` package is not ready for Python3
================================ 14 passed, 1 skipped, 304 deselected, 1 warning in 1.06s =================================

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, probably missed to spot the point. Thanks for clarifying!

@amotl amotl merged commit 6a24a8a into mqtt-tools:main Jul 24, 2023
10 of 12 checks passed
@jhakonen jhakonen deleted the feat_variable_expansion_pr branch July 24, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants