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

New EMC VPLEX module to create storage-volume. #55512

Open
wants to merge 12 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@hwakabh
Copy link

commented Apr 18, 2019

SUMMARY

New modules to manage storage-volume on EMC VPLEX.
Allows creating storage-volume encapsulation from backend storage-array.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

storage/emc/emc_vplex_storagevolume

ADDITIONAL INFORMATION
  • VPLEX storage-volume encapsulation should meet the requirements below:
    • Some storage-array supported are properly exposed to VPLEX with backend zoning
    • LUN(s) are already created on backend storage-array and exported to VPLEX
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

@remixtj

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

@remixtj

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

I've looked through the module and i see several usage of logging. I don't know if it is a good practice, and anyway i'd prefer to have logging set as optional. Adding some notes now to the code.

@hwakabh

This comment has been minimized.

Copy link
Author

commented Apr 20, 2019

@remixtj Thanks to taking your time and reviewing codes. As pointed out, modified the module's behavior when it hit some errors, and made codes allowed user to provide path of this module logs.

@remixtj

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

other notes: the module is not idempotent, because if the storage-volume, with the given setup is already present it fails. Instead, you should check if the volume you're trying to create already exists with the configuration you put as input. Additionally, there's no state argument, so user cannot remove a storage volume (with state: absent as example)

@ansibot ansibot added the stale_ci label May 1, 2019

@hwakabh

This comment has been minimized.

Copy link
Author

commented May 5, 2019

Closed and reopened PR for retesting by CI.

@hwakabh hwakabh closed this May 5, 2019

@hwakabh hwakabh reopened this May 5, 2019

@ansibot ansibot removed the stale_ci label May 5, 2019

@remixtj

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Seems good now but i didn't tested because i have no VPLEX devices to test against now.

@hwakabh

This comment has been minimized.

Copy link
Author

commented May 7, 2019

@remixtj
Thanks to your comments, and reviewing.

As my understanding, source codes for new ansible modules in new contributor's PRs would be tested with both CI-testings(by Shippable) and existing contributor's reviews, and these are not including unit-testings.

Considering the resources which seems to be vendor-specific like this PR, how do the new source codes been merged to master branch ??
Is it enough to note here the evidences, such as logs, of unit-testings in private environment ??

@ansibot ansibot added the stale_ci label May 16, 2019

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.