Add entrypoints for moved modules #17
Conversation
|
I applied formatting in #20 and rebased this change, dropping the pre-commit's conflicting commit. |
chrismeyersfsu
force-pushed
the
entrypoints2
branch
from
07fb656
to
24aee5c
Compare
There was a problem hiding this comment.
I imagine that
https://github.com/ansible/awx-plugins/blob/4126dc7/tests/importable_test.py is coupled tightly enough to warrant being in the scope. Could you also update the smoke test using @pytest.mark.parametrize? If you want to keep some preparatory code in the test, add pytest-subtests to dependencies/direct/py.in and inject the subtests fixture to mark each of the cases in the test report correctly. Though, the same could be achieved by putting that into the own parametrized fixture with @pytest.fixture(params=[]). Multiple parametrizations can be stacked. Feel free to ask me if you need pointers.
chrismeyersfsu
force-pushed
the
entrypoints2
branch
2 times, most recently
from
6e23df4
to
803a090
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
|
I dropped the automerge until the next review. |
chrismeyersfsu
force-pushed
the
entrypoints2
branch
6 times, most recently
from
5d2b347
to
7218e41
Compare
| conjur = "awx_plugins.credentials.conjur:conjur_plugin" | ||
| hashivault_kv = "awx_plugins.credentials.hashivault:hashivault_kv_plugin" | ||
| hashivault_ssh = "awx_plugins.credentials.hashivault:hashivault_ssh_plugin" | ||
| azure_kv = "awx_plugins.credentials.azure_kv:azure_keyvault_plugin" | ||
| aim = "awx_plugins.credentials.aim:aim_plugin" | ||
| centrify_vault_kv = "awx_plugins.credentials.centrify_vault:centrify_plugin" | ||
| thycotic_dsv = "awx_plugins.credentials.dsv:dsv_plugin" | ||
| thycotic_tss = "awx_plugins.credentials.tss:tss_plugin" | ||
| aws_secretsmanager_credential = "awx_plugins.credentials.aws_secretsmanager:aws_secretmanager_plugin" |
There was a problem hiding this comment.
Is the inventory plugin missing from here?
tests/importable_test.py
Outdated
|
|
||
| assert callable(entry_points['x'].load()) | ||
| assert expected_entry_point_name in entry_points.names | ||
| assert entry_points[expected_entry_point_name].value == f'{entry_points_group}.{expected_entry_point_module}:{expected_entry_point_func}' |
There was a problem hiding this comment.
Technically, the data here is not the entry_points_group. It just happens to be the same string, but it's actually coming from the importable.
tests/importable_test.py
Outdated
|
|
||
| import pytest | ||
|
|
||
|
|
||
| EXPECTED_ENTRY_POINTS: list[tuple[str, str, str]] = [ | ||
| ('awx_plugins.credentials', 'aim', 'aim', 'aim_plugin'), |
There was a problem hiding this comment.
The last components two are only used in the same place and concatenated always. No need to present them as disconnected inputs, because they are tightly coupled. Plus, taking into account that they are joined to the rest of the import path, I'd say the entire spec should be a param as a single unit.
| ('awx_plugins.credentials', 'aim', 'aim', 'aim_plugin'), | |
| ('awx_plugins.credentials', 'aim', 'awx_plugins.credentials.aim:aim_plugin'), |
Additionally, reusing the 'awx_plugins.credentials' string this many times will 💯 trip the WPS rule violation. It'd need to go into a constant.
Seeing that it's all the same in every param, I'd maybe move it into a separate @pytest.mark.parametrize, stacking two decorators together.
OTOH, if the observation of missing inventory plugin references is addressed, they'd need to have this structure. But still, this could be simplified a bit via something like itertools.product().
| "azure-identity", # credentials.azure_kv | ||
| "azure-keyvault", # credentials.azure_kv | ||
| "boto3", # credentials.awx_secretsmanager | ||
| "msrestazure", # credentials.azure_kv | ||
| "python-dsv-sdk >= 1.0.4", # credentials.thycotic_dsv | ||
| "python-tss-sdk >= 1.2.1", # credentials.thycotic_tss | ||
| "requests", # credentials.aim, credentials.centrify_vault, credentials.conjur, credentials.hashivault |
There was a problem hiding this comment.
I have a few things to say here:
- Q: Are all of these mandatory in all installs? Can individual plugins be invoked w/o depending on others? If yes, maybe the deps should be put into a series of extras through
optional-dependenciesrather than unconditional ones. - ACTION REQUIRED: https://github.com/ansible/awx-plugins/actions/runs/10599668926/job/29375336003?pr=17#step:17:61 revealed that the
awxdependency is missing. It should be listed somewhere. Perhaps, in an extra, depending on what's decided above.
chrismeyersfsu
force-pushed
the
entrypoints2
branch
7 times, most recently
from
a685285
to
a14e4c5
Compare
tests/importable_test.py
Outdated
| ) | ||
| def test_entry_points_exposed(entry_points_group: str) -> None: | ||
| """Verify the plugin entry point is discoverable. | ||
| class TestEntryPoints: |
There was a problem hiding this comment.
Could you avoid introducing classes? They only complicate things. The tests are already namespaced by being in modules.
If you want to reuse parametrization, you can assign the marker to a variable and use it.
As in
with_credential_entry_points = pytest.mark.parametrize(...)
And then, apply @with_credential_entry_points to the tests w/o creating another layer of indirection.
webknjaz
force-pushed
the
entrypoints2
branch
3 times, most recently
from
55fce20
to
1e96a2f
Compare
These are coming from `awx` having convoluted and undeclared dependency tree.
This patch includes tests for loading them.
|
I've decided to suppress some of the import errors in the inventory plugins as they aren't well-integrated anyway. This lets us unblock the docs builds among other things. The dependencies are added all together and pulled in unconditionally. I think it might make sense to split them into optional ones (extras). I didn't add The inventory plugins are exposed as classes while the credential ones are initialized objects. We need to define a consistent interface for all the plugins. |
No description provided.