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

Allow proxy minion types to be dynamically loaded #50183

Merged
merged 7 commits into from
Nov 16, 2018

Conversation

cro
Copy link
Contributor

@cro cro commented Oct 23, 2018

What does this PR do?

This PR creates a new loadable module type called a metaproxy and abstracts the existing proxy minion to become a type of metaproxy. This enables us to build different types of proxy minions that can still load existing proxymodules.

Tests written?

No, but existing tests for proxy minions pass successfully.

Commits signed with GPG?

Yes

@cro cro requested a review from a team as a code owner October 23, 2018 20:10
@ghost ghost self-requested a review October 23, 2018 20:10
Copy link
Contributor

@gtmanfred gtmanfred left a comment

Choose a reason for hiding this comment

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

instead of having these larger if statements, can we just put those

if opts is None:
    opts = __opts__

At the top?

if not opts:
ckminions = salt.utils.minions.CkMinions(__opts__)
else:
ckminions = salt.utils.minions.CkMinions(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just make this ckminions = salt.utils.minions.CkMinions(opts or __opts__) and get rid of the rest of the if statement?

if not opts:
ckminions = salt.utils.minions.CkMinions(__opts__)
else:
ckminions = salt.utils.minions.CkMinions(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@rallytime
Copy link
Contributor

@cro This has several lint failures as well as some test failures. Can you take a look?

@rallytime
Copy link
Contributor

@cro Still some proxy test failures here, I'm afraid.

https://jenkinsci.saltstack.com/job/pr-kitchen-ubuntu1604-py2/job/PR-50183/7/

@cro
Copy link
Contributor Author

cro commented Oct 29, 2018

Yes, looks like they are related to the ProxyCaller. I am looking more closely at it now.

@cro cro force-pushed the metaproxy branch 2 times, most recently from 796ce64 to d4c15f7 Compare October 31, 2018 14:58
@cro cro closed this Oct 31, 2018
@cro cro reopened this Oct 31, 2018
@gtmanfred gtmanfred closed this Oct 31, 2018
@gtmanfred gtmanfred reopened this Oct 31, 2018
@cro cro force-pushed the metaproxy branch 3 times, most recently from 174e1de to e35eb6a Compare November 6, 2018 17:39
@cro cro force-pushed the metaproxy branch 2 times, most recently from a89d442 to 12c147a Compare November 13, 2018 16:56
if not opts:
opts = __opts__
nodegroups = opts.get('nodegroups', {})
matchers = salt.loader.matchers(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you added a __matchers__ dunder to the packed items, similar to what we do for __salt__ in minion_mods, then you don't need to instantiate a new LazyLoader here. I get that we might still need to do it, in cases where actual opts are passed in, but it shouldn't be necessary in most cases.

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.

5 participants