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

StorageSystemModule for HPE OneView #32801

Open
wants to merge 11 commits into
base: devel
from

Conversation

@ricardogpsf

ricardogpsf commented Nov 10, 2017

SUMMARY

Added new oneview_storage_system module for managing HPE OneView Storage Systems resource and unit tests.

Related issue for more information: HPE OneView support

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

oneview_storage_system

ANSIBLE VERSION
ansible --version
ansible 2.5.0
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/site-packages/ansible-2.5.0-py2.7.egg/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.14 (default, Oct 10 2017, 02:49:49) [GCC 4.9.2]
ADDITIONAL INFORMATION

Note: to run the example you need an Oneview Appliance.
Example of usage:

- name: Add a Storage System with one managed pool (before API500)
  oneview_storage_system:
    hostname: 172.16.101.48
    username: administrator
    password: my_password
    api_version: 300
    state: present
    data:
        credentials:
            ip_hostname: 172.18.11.12
            username: username
            password: password
        managedDomain: TestDomain
        managedPools:
          - domain: TestDomain
            type: StoragePoolV2
            name: CPG_FC-AO
            deviceType: FC

  delegate_to: localhost

- name: Add a StoreServ Storage System with one managed pool (API500 onwards)
  oneview_storage_system:
    hostname: 172.16.101.48
    username: administrator
    password: my_password
    api_version: 500
    state: present
    data:
      credentials:
          username: username
          password: password
      hostname: 172.18.11.12
      family: StoreServ
      deviceSpecificAttributes:
          managedDomain: TestDomain
          managedPools:
              - domain: TestDomain
                name: CPG_FC-AO
                deviceType: FC

  delegate_to: localhost

- name: Remove the storage system by its IP (before API500)
  oneview_storage_system:
    hostname: 172.16.101.48
    username: administrator
    password: my_password
    api_version: 300
    state: absent
    data:
        credentials:
            ip_hostname: 172.18.11.12

  delegate_to: localhost

- name: Remove the storage system by its IP (API500 onwards)
  oneview_storage_system:
    hostname: 172.16.101.48
    username: administrator
    password: my_password
    api_version: 500
    state: absent
    data:
        credentials:
            hostname: 172.18.11.12

  delegate_to: localhost
@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 10, 2017

The test ansible-test sanity --test pep8 [?] failed with the following error:

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py:195:25: W601 .has_key() is deprecated, use 'in'

click here for bot help

@ricardogpsf ricardogpsf force-pushed the ricardogpsf:hpe-oneview/storage-system-module branch Nov 13, 2017

@ansibot ansibot removed the ci_verified label Nov 13, 2017

@ricardogpsf ricardogpsf force-pushed the ricardogpsf:hpe-oneview/storage-system-module branch Nov 13, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 13, 2017

@adriane-cardozo @fgbulsoni @tmiotto

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

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py Outdated
- C(present) will ensure data properties are compliant with OneView.
- C(absent) will remove the resource from OneView, if it exists.
choices: ['present', 'absent']
required: true

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

Should default to present instead of being required

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py Outdated
def __init__(self):
argument_spec = dict(
state=dict(
required=True,

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

Should be default='present' and this whole state should fit into one line.
I also suggest reordering the argument spec as mentioned in here

test/units/modules/remote_management/oneview/test_oneview_storage_system.py Outdated
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
import yaml
from ansible.compat.tests import unittest, mock

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

Suggestion: add a blank line here to separate import X and from X import types of import

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py Outdated
required=True,
choices=['present', 'absent']
),
state=dict(default='present', choices=['present', 'absent']),

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

This is missing the mandatory type.
As I mentioned before, I'd also recommend reorganizing everything inside the argument_spec as mentioned here.
as in: state=(type='str', default='present', choices=['absent', 'present']) .

Since type is mandatory, make it come first, then default or required and after that the rest.

This comment has been minimized.

@ricardogpsf

ricardogpsf Nov 13, 2017

ah, sorry. My bad. =)

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py Outdated
data=dict(required=True, type='dict')
)
super(StorageSystemModule, self).__init__(additional_arg_spec=argument_spec,
super(StorageSystemModule, self).__init__(additional_arg_spec=self.argument_spec,
validate_etag_support=True)

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

put validate_etag_support=True) in line 145, there's no need for this split

lib/ansible/modules/remote_management/oneview/oneview_storage_system.py Outdated
@@ -137,12 +137,13 @@ class StorageSystemModule(OneViewModuleBase):
"(credentials.ip_hostname if API version lower than 500 )."
MSG_CREDENTIALS_MANDATORY = "The attribute 'credentials' is mandatory for Storage System creation."
argument_spec = dict(
state=dict(type='str', default='present', choices=['present', 'absent']),

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 13, 2017

Contributor

choices=['absent', 'present']

@fgbulsoni

This comment has been minimized.

Contributor

fgbulsoni commented Nov 13, 2017

LGTM, shipit

@adriane-cardozo

This comment has been minimized.

adriane-cardozo commented Nov 13, 2017

shipit

@ansibot ansibot added shipit and removed community_review labels Nov 13, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 20, 2017

The test ansible-test sanity --test pep8 [?] failed with the following error:

test/units/modules/remote_management/oneview/test_oneview_storage_system.py:218:40: E127 continuation line over-indented for visual indent

click here for bot help

@ricardogpsf ricardogpsf force-pushed the ricardogpsf:hpe-oneview/storage-system-module branch Nov 21, 2017

test/units/modules/remote_management/oneview/test_oneview_storage_system.py Outdated
import yaml
from ansible.compat.tests import mock
from hpe_test_utils import mock_ov_client, mock_ansible_module

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 21, 2017

Contributor

It is considered a bad practice to import pytest fixtures, I'm also seeing several flake errors commenting that these are being rewritten probably because of this.

test/units/modules/remote_management/oneview/hpe_test_utils.py Outdated
from mock import Mock, patch
from oneview_module_loader import ONEVIEW_MODULE_UTILS_PATH
from hpOneView.oneview_client import OneViewClient
@pytest.fixture

This comment has been minimized.

@fgbulsoni

fgbulsoni Nov 21, 2017

Contributor

These fixtures should likely be inside conftest.py

This comment has been minimized.

@ricardogpsf
@fgbulsoni

This comment has been minimized.

Contributor

fgbulsoni commented Nov 21, 2017

LGTM

1 similar comment
@tmiotto

This comment has been minimized.

tmiotto commented Nov 22, 2017

LGTM

@ricardogpsf ricardogpsf force-pushed the ricardogpsf:hpe-oneview/storage-system-module branch to 3c0ae8f Nov 28, 2017

@ansibot ansibot removed the needs_rebase label Nov 28, 2017

@mattclay

This comment has been minimized.

Member

mattclay commented Dec 1, 2017

CI failure in unit tests:

>       self.resource = getattr(mock_ov_client, "%s" % (marker.args))
E       TypeError: not all arguments converted during string formatting

test/units/modules/remote_management/oneview/hpe_test_utils.py:31: TypeError
@Akasurde

This comment has been minimized.

Member

Akasurde commented Nov 16, 2018

@ricardogpsf Could you please rebase this branch ?

@ricardogpsf

This comment has been minimized.

ricardogpsf commented Nov 21, 2018

@Akasurde I'm not working on it anymore.
I'am marking @soodpr and @madhav-bharadwaj because they could say something about of this task. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment