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

Add new Cloudstack module cs_image_store #53617

Merged
merged 13 commits into from Mar 16, 2019

Conversation

Projects
None yet
5 participants
@PatTheSilent
Copy link
Contributor

PatTheSilent commented Mar 11, 2019

SUMMARY

Add a new Cloudstack module cs_image_store.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

cs_image_store

ADDITIONAL INFORMATION

The module is capable of adding or removing Cloudstack Image Stores. Updating is not supported as the Cloudstack API doesn't support it.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

@PatTheSilent PatTheSilent marked this pull request as ready for review Mar 13, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 13, 2019

@DazWorrall @dpassante @jeffersongirao @marcaurele @netservers @onitake @rhtyd

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@dpassante
Copy link
Contributor

dpassante left a comment

Thanks @PatTheSilent for your contribution!

For consistency, I would suggest some changes in the documentation, such as "2 spaces" indentation or the addition of parameter types (you can use cs_instance as an example), but I will make these changes on a PR separate if you prefer.

Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
Show resolved Hide resolved test/integration/targets/cs_image_store/tasks/main.yml
Show resolved Hide resolved test/integration/targets/cs_image_store/tasks/main.yml Outdated
Show resolved Hide resolved test/integration/targets/cs_image_store/tasks/main.yml Outdated
Show resolved Hide resolved test/integration/targets/cs_image_store/tasks/main.yml Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
Show resolved Hide resolved lib/ansible/modules/cloud/cloudstack/cs_image_store.py Outdated
self.query_api('addImageStore', **args)
else:
if self.has_changed(args, image_store):
self.fail_json(msg='Image Store cannot be updated. Please delete it first.')

This comment has been minimized.

@resmo

resmo Mar 13, 2019

Member

I don't like to fail at this point. There are way better options to handle this, we could implement a new force param to let a user "remove and add" an new image store by the users intention and if not force, just show a warning message to the users that we can not change the image store, but would recreate on force. We already have such a logic for in the cs_instance module for changing the offering on a running instance.

This comment has been minimized.

@PatTheSilent

PatTheSilent Mar 14, 2019

Author Contributor

Yeah, I agree that this could've been handled better. To be clear I did consider such an approach but was not sure if "recreate on update" is the right way. Adding a force param does seem like a good idea. I'll go ahead and implement it.

This comment has been minimized.

@PatTheSilent

PatTheSilent Mar 14, 2019

Author Contributor

This is done. @resmo can you please check?

@PatTheSilent

This comment has been minimized.

Copy link
Contributor Author

PatTheSilent commented Mar 14, 2019

Thanks @PatTheSilent for your contribution!

For consistency, I would suggest some changes in the documentation, such as "2 spaces" indentation or the addition of parameter types (you can use cs_instance as an example), but I will make these changes on a PR separate if you prefer.

Thanks, @dpassante for pointing that out. I'd rather handle that in the scope of this PR if you don't mind.

@dpassante
Copy link
Contributor

dpassante left a comment

Good job! @PatTheSilent

Some additional points. I'd also suggest adding some more assert tests to check the values returned by the module.

Update cs_image_store documentation
* Add examples of Store deletion and recreation
* Correct the example of creating an Image Store (lacking param)
* Cleanup newlines and unneeded quotes
@PatTheSilent

This comment has been minimized.

Copy link
Contributor Author

PatTheSilent commented Mar 15, 2019

@dpassante I'll add the asserts in the next commit.

@dpassante

This comment has been minimized.

Copy link
Contributor

dpassante commented Mar 15, 2019

@PatTheSilent I forgot to mention that by convention we specify in the examples that the tasks must be delegated to localhost. Could you please add delegate_to: localhost in examples?

@PatTheSilent

This comment has been minimized.

Copy link
Contributor Author

PatTheSilent commented Mar 15, 2019

@dpassante both topics are done now.

@dpassante

This comment has been minimized.

Copy link
Contributor

dpassante commented Mar 15, 2019

Great!

@dpassante
Copy link
Contributor

dpassante left a comment

shipit

@resmo

resmo approved these changes Mar 16, 2019

Copy link
Member

resmo left a comment

There is a missing check_mode test for absent but can't hold this PR back for this. Please create a separate PR for this additional test case.

shipit

@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 16, 2019

great stuff!

@resmo resmo merged commit 851cfc0 into ansible:devel Mar 16, 2019

1 check passed

Shippable Run 114131 status is SUCCESS.
Details
@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 16, 2019

@dpassante Thanks david for the perfect reviews!

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.