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

Support loading entrypoints by passing a module instead of a function #58943

Merged
merged 8 commits into from Mar 5, 2021

Conversation

s0undt3ch
Copy link
Member

@s0undt3ch s0undt3ch commented Nov 13, 2020

What does this PR do?

Support loading entrypoints by passing a module instead of a function.

The old way of defining salt loader entrypoints was, for example, on setup.cfg:

[options.entry_points]
salt.loader=
  runner_dirs = thirpartypackage.loader:func_to_get_list_of_dirs
  module_dirs = thirpartypackage.loader:func_to_get_list_of_dirs

This old way is still supported in the rare ocasion where you actually
want to pass multiple paths to the loader and to not break backwards
compatibility.

The new way of defining the salt loader entrypoints is, for example, on setup.cfg:

[options.entry_points]
salt.loader=
  <this-name-does-not-matter = thirpartypackage
  <this-name-does-not-matter = thirpartypackage:callable

With this new approach, when the target is a package, Salt looks for the
ext_type being loaded as submodule of the package and if it exists, it
loads from there.
If the target is a callable, then, that callable should return a
dictionary where the keys are the several salt loader names. For
salt execution modules it would be modules, for salt state modules it would
be states, etc. The same directory layout you see in salt.
The values for those keys needs to be an iterable(not a string which is
also an iterable), ie, a list, a generator, etc, whose values are the
paths from where to load the salt loader modules.

What issues does this PR fix or reference?

Fixes #58939

@s0undt3ch s0undt3ch added the Aluminium Release Post Mg and Pre Si label Nov 13, 2020
@s0undt3ch s0undt3ch added this to the Aluminium milestone Nov 13, 2020
@s0undt3ch s0undt3ch requested a review from dwoz November 13, 2020 16:48
@s0undt3ch s0undt3ch requested a review from a team as a code owner November 13, 2020 16:48
@s0undt3ch s0undt3ch force-pushed the hotfix/entry-points branch 4 times, most recently from 96f5192 to 7d8c22a Compare November 20, 2020 06:58
waynew
waynew previously requested changes Nov 23, 2020
changelog/58939.added Outdated Show resolved Hide resolved
@s0undt3ch s0undt3ch force-pushed the hotfix/entry-points branch 3 times, most recently from 404e85e to 7b56830 Compare January 25, 2021 19:11
@s0undt3ch s0undt3ch requested a review from waynew January 25, 2021 19:14
@s0undt3ch s0undt3ch force-pushed the hotfix/entry-points branch 3 times, most recently from 8ea491a to f39d4fe Compare February 26, 2021 12:32
@dwoz
Copy link
Contributor

dwoz commented Feb 26, 2021

[options.entry_points]
salt.loader=
  <this-name-does-not-matter = thirpartypackage
  <this-name-does-not-matter = thirpartypackage:callable

I think this-name-does-not-matter is part of what we want to show up in the versions report, in which case it would matter. The reason being a pypi package might contain more than one 'extension'.

[options.entrypoints]
salt.loader=
   myplugin1 = myplugin1module
   myplugin2 = myplugin2module

At least, I believe this is what was intended for they keys in the configs for entrypoints and can see some value from it.

Then in the versions report we could show something like

third-party-extension[myplugin1]
third-party-extension[myplugin2]

or simply

myplugin1
myplugin2

Not sure what's really better there..

I could see that in most cases there would be a single plugin and the name would match the package name.

@s0undt3ch
Copy link
Member Author

But then you have the same extension, with the same version, showing up multiple times on the versions report, just because they provide more than one entry point.... I don't see any value added there....

third-party-extension[myplugin1,myplugin2,etc...]

Would be less verbose, but I still don't see added value there....

twangboy
twangboy previously approved these changes Feb 26, 2021
@s0undt3ch
Copy link
Member Author

re-run all

@dwoz
Copy link
Contributor

dwoz commented Mar 2, 2021

But then you have the same extension, with the same version, showing up multiple times on the versions report, just because they provide more than one entry point.... I don't see any value added there....

third-party-extension[myplugin1,myplugin2,etc...]

Would be less verbose, but I still don't see added value there....

It's not about the extension, it's about the plugin. There can be a case where the package providing the plugin has a whole lot of functionality that has nothing to do with Salt. For instance, boto could provide a salt plugin that they maintain which ships with boto3.

@s0undt3ch
Copy link
Member Author

It's not about the extension, it's about the plugin. There can be a case where the package providing the plugin has a whole lot of functionality that has nothing to do with Salt. For instance, boto could provide a salt plugin that they maintain which ships with boto3.
And?

We only report salt related extensions...

I'm not seeing the issue

@s0undt3ch
Copy link
Member Author

For instance, boto could provide a salt plugin that they maintain which ships with boto3.

And we report that boto3 version X.Y.Z provide extensions for salt...

dwoz
dwoz previously approved these changes Mar 2, 2021
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

I'll look into this closer tomorrow morning, but in the meantime there is the changelog comment

changelog/58938.added Outdated Show resolved Hide resolved
salt/loader.py Show resolved Hide resolved
salt/loader.py Show resolved Hide resolved
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Hey! Sorry this took a bit longer than planned to get back to you. I think this looks excellent!

There are a couple of places where we could add a bit more testing (the negative cases). But otherwise this seems 👍 to me.

I cloned this locally and added my own "extension module" and tried the 3 different styles of loading - list, dict, and ext_types sub module + /modules/ dir and manually verified that it works.

Oh, one more question: should we also test the other types of dirs? I see that this test includes modules, but don't we have other potential dirs? from my debugging code:

ext_dirs? auth_dirs
ext_dirs? engines_dirs
ext_dirs? fileserver_dirs
ext_dirs? grains_dirs
ext_dirs? log_handlers_dirs
ext_dirs? matchers_dirs
ext_dirs? module_dirs
ext_dirs? returner_dirs
ext_dirs? runner_dirs
ext_dirs? serializers_dirs
ext_dirs? tokens_dirs
ext_dirs? top_dirs
ext_dirs? utils_dirs
ext_dirs? wheel_dirs

salt/loader.py Show resolved Hide resolved
tests/support/pytest/helpers.py Show resolved Hide resolved
tests/support/pytest/helpers.py Show resolved Hide resolved
@s0undt3ch s0undt3ch requested a review from krionbsd March 5, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aluminium Release Post Mg and Pre Si
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Salt's Entry Points should not need to be a callable
6 participants