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

use __utils__ in salt.cloud #35483

Merged
merged 1 commit into from
Aug 18, 2016
Merged

use __utils__ in salt.cloud #35483

merged 1 commit into from
Aug 18, 2016

Conversation

gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Aug 16, 2016

What does this PR do?

Use __utils__ in salt.cloud instead of importing salt.utils

Use this for sock_dir and cachedir instead of using syspaths

What issues does this PR fix or reference?

Closes #18419 and #34806

Should also revert #34870 when this is merged to develop, because it is not needed now that we will have opts available in the cloud utils.

Tests written?

No

@gtmanfred gtmanfred force-pushed the 2015.8 branch 3 times, most recently from ae8045b to 15ab9b6 Compare August 16, 2016 17:02
@@ -339,7 +339,7 @@ def list_nodes_full(call=None):
provider = comps[0]

__opts__['update_cachedir'] = True
salt.utils.cloud.cache_node_list(ret, provider, __opts__)
__utils__['cloud.cache_node_list'](ret, provider, __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 I'm not mistaken, since this is now being loaded by the loader we should have __opts__ already packed into utils.If so, we should go ahead and modify these utility functions to simply use the copy of opts they already have, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left that in just in case someone else was using it for one of their functions in reference to something.

And then we can remove it in Carbon, instead of making a change to the function that we don't need to make back in an older release. But I can remove the opts if that is what should be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. If you want to do a deprecation path, we can do that but let's just go ahead and merge this as it is. Thanks.

@cachedout
Copy link
Contributor

Left one comment in-line about optimizing this further by just using the copy of opts that we should already have loaded into the utils function, unless there is a reason to override it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants