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

CloudStack modules: rename _facts -> _info #61090

Merged
merged 17 commits into from Aug 29, 2019

Conversation

@felixfontein
Copy link
Contributor

commented Aug 22, 2019

SUMMARY

Fixes #60515, fixes #53916.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cs_instance_facts
cs_zone_facts

@ansibot

This comment has been minimized.

@ansibot

This comment was marked as off-topic.

Copy link
Contributor

commented Aug 22, 2019

@felixfontein this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@resmo

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

I though about changing the modules to return lists: list of zones, list of instances, with the possibility to filter by name to only get one item (but still a list). @dpassante any thoughts?

@ansibot ansibot removed the needs_triage label Aug 22, 2019

@samccann
Copy link
Contributor

left a comment

docs portion LGTM

@dpassante

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@resmo I think it would be an interesting feature. However, returning a list for filtered items may break existing deployments that expect a dictionary.
These impacts could be mitigated by keeping the cloudstack_zone / cloudstack_instance exposure in the ansible_facts when there is only one item.

I also thought about writing a meta module that exposes lists of resources with the ability to filter by type of resources and by name to not break the existing but it seemed redundant with other modules and time-consuming to write and to maintain.

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@dpassante if the return values should be modified, the old module _facts modules should be deprecated and new _info modules added (with the new return values). Existing playbooks using the _facts version should continue to run (with deprecation warning, and until _facts is gone).

@dpassante

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@felixfontein Indeed. So, it's a good time to change the values returned by the new _info module!

@ansibot ansibot removed the needs_rebase label Aug 29, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

CS tests run through. Sanity tests should be fixed with the last commits.

ready_for_review

@dpassante
Copy link
Contributor

left a comment

LGTM

@resmo

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

shipit

@resmo

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Fixes #53916

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Final commits. Fix version number for deprecation and add examples.

@resmo

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

shipit

@dpassante

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Thanks @felixfontein for these changes.

@dpassante

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

shipit

@felixfontein felixfontein merged commit 0c592c8 into ansible:devel Aug 29, 2019

1 check passed

Shippable Run 140979 status is SUCCESS.
Details

@felixfontein felixfontein deleted the felixfontein:info-cloudstack branch Aug 29, 2019

@felixfontein

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@samccann @s-hertel @dpassante @resmo thank you all for helping getting this done! :)

adharshsrivatsr added a commit to adharshsrivatsr/ansible that referenced this pull request Sep 3, 2019
CloudStack modules: rename _facts -> _info (ansible#61090)
* Rename cloudstack _facts -> _info

* Add changelog.

* Fix errors due to overloaded get_result having different parameter names.

* Fix examples.

* Change debug tasks.

* Remove unneeded code.

* Change from rename -> deprecate+new module.

* Make cs_zone_info return a list.

* Make cs_instance_info return a list.

* Fix return value docs.

* Fix typo.

* Improve tests.

* Fix cs_zone_info.

* Linting.

* Fix alias/option switch.

* Fix version numbers.

* Extend docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.