Skip to content

Add new glance module and states based on shade#44817

Merged
rallytime merged 3 commits intodevelopfrom
unknown repository
Dec 5, 2017
Merged

Add new glance module and states based on shade#44817
rallytime merged 3 commits intodevelopfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Dec 4, 2017

What does this PR do?

This PR adds a new module (glanceng.py) to supercede glance.py. It uses the shade library from the openstack project (https://github.com/openstack-infra/shade.git) Shade is a project that has committed to maintaining the same API forever. It may add new features and more efficient ways of doing things, but it will never remove things.

We are making extensive use of **kwargs to help prevent the need to maintain lots of duplicate logic that exists in shade itself.

What issues does this PR fix or reference?

N/A

Tests written?

Not yet

@rallytime rallytime requested a review from gtmanfred December 4, 2017 20:48
@gtmanfred gtmanfred added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 4, 2017
Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

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

Thank you @SamYaple. I have a couple of comments. Once those are addressed we can get this in.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be Oxygen instead of Nitrogen. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The salt.utils functions have been deprecated and moved to new files recently. We're moving away from importing salt.utils directly. Please use the new paths for any utils functions you may need.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are using salt.utils.clean_kwargs for backwards compatibility with this

Copy link
Contributor

Choose a reason for hiding this comment

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

Backwards compatibility with what? This PR is being opened against develop.

Copy link
Contributor

@gtmanfred gtmanfred Dec 5, 2017

Choose a reason for hiding this comment

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

some people will want to drop this into their already functioning 2017.7 setup.

Since right now the keystone stuff in 2017.7 and before doesn't work with openstack keystone v3

Copy link
Contributor

Choose a reason for hiding this comment

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

Oxygen here, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

please change this to __utils__['args.clean_kwargs']

Copy link
Author

Choose a reason for hiding this comment

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

should I just use the new method and not make it backwards compat? we will still have to carry these internally anyway. itsnot a big deal to me.

@ghost
Copy link
Author

ghost commented Dec 4, 2017

I made the changes against the just-merged-yesterday keystone module and states as well so that doesnt get lost

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use salt.utils. It will create unnecessary deprecation warnings. This entire try block should be rewritten as either

return __utils__['args.clean_kwargs'](**kwargs)

or

return salt.utils.args.clean_kwargs(**kwargs)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please replace this entire block with either

return __utils__['args.clean_kwargs'](**kwargs)

or

return salt.utils.args.clean_kwargs(**kwargs)

The develop branch has deprecated the use of salt.utils and it will generate a deprecation warning if used.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be importing salt.utils anymore in develop.

Copy link
Author

Choose a reason for hiding this comment

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

this was already commented on above. @gtmanfred what should I be doing here?

@rallytime rallytime merged commit 1a75713 into saltstack:develop Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants