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 module cpm_status for WTI device management #42970

Merged
merged 18 commits into from Aug 21, 2018
Merged

Conversation

syncpak
Copy link
Contributor

@syncpak syncpak commented Jul 18, 2018

SUMMARY

Added Western Telematic Inc. (WTI) module allowing communication with WTI OOB and PDU devices via RESTful calls and http/https
Add module cpm_status for WTI device management

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

cpm_status

ANSIBLE VERSION
ansible 2.7.0.dev0 (WTIDevice 177a0d0a8e) last updated 2018/07/18 10:43:00 (GMT -700)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/kenp/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/kenp/ansible/lib/ansible
  executable location = /home/kenp/ansible/bin/ansible
  python version = 2.7.6 (default, Nov 23 2017, 15:49:48) [GCC 4.8.4]

ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2018

The test ansible-test sanity --test pep8 [explain] failed with 12 errors:

lib/ansible/module_utils/network/cpm/cpm_common.py:48:5: E128 continuation line under-indented for visual indent
lib/ansible/module_utils/network/cpm/cpm_common.py:51:5: E128 continuation line under-indented for visual indent
lib/ansible/module_utils/network/cpm/cpm_common.py:52:5: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:109:20: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:133:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:134:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:137:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:138:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:141:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:142:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:145:9: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:146:9: E128 continuation line under-indented for visual indent

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/network/cpm/cpm_status.py:0:0: E101 Interpreter line is not "#!/usr/bin/python"

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2018

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 ci_verified Changes made in this PR are causing tests to fail. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. networking Network category new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 18, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 18, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2018

The test ansible-test sanity --test pep8 [explain] failed with 12 errors:

lib/ansible/module_utils/network/cpm/cpm_common.py:48:19: E128 continuation line under-indented for visual indent
lib/ansible/module_utils/network/cpm/cpm_common.py:51:5: E128 continuation line under-indented for visual indent
lib/ansible/module_utils/network/cpm/cpm_common.py:52:30: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:110:33: E127 continuation line over-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:134:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:135:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:138:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:139:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:142:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:143:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:146:14: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:147:14: E128 continuation line under-indented for visual indent

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 18, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2018

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/module_utils/network/cpm/cpm_common.py:50:5: E128 continuation line under-indented for visual indent
lib/ansible/modules/network/cpm/cpm_status.py:132:161: E501 line too long (173 > 160 characters)
lib/ansible/modules/network/cpm/cpm_status.py:134:161: E501 line too long (170 > 160 characters)
lib/ansible/modules/network/cpm/cpm_status.py:136:161: E501 line too long (168 > 160 characters)
lib/ansible/modules/network/cpm/cpm_status.py:138:161: E501 line too long (168 > 160 characters)

click here for bot help

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 18, 2018
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jul 19, 2018
@syncpak syncpak changed the title Add module cps_status for WTI device management Add module cpm_status for WTI device management Jul 22, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 30, 2018
@syncpak
Copy link
Contributor Author

syncpak commented Jul 31, 2018

Hi everyone, I know this module is a bit difficult to test/comment/approve because it applies our company produced networking equipment, I setup a community accessible unit if anyone would like to try this module. The excerpt from my /etc/ansible/hosts file and a sample playbook are below.
I apologize if we are breaking convention, we are just trying to help the process.

(From /etc/ansible/hosts file)

[wti_demo]
WTI-REST ansible_host=rest.wti.com

[wti_demo:vars]
ansible_user=rest
ansible_pw=restfulpassword
use_https=True

# ansible-playbook ./getver.yml
- name: Get Device Firmware version
  connection: local
  hosts: wti_demo
  gather_facts: false

  tasks:
  - set_fact: use_https=True
    when: use_https is undefined

  - name: run Get Device Firmware version
    cpm_status:
      cpm_action: "firmware"
      cpm_url: "{{ansible_host}}"
      cpm_username: "{{ansible_user}}"
      cpm_password: "{{ansible_pw}}"
      use_https: "{{use_https}}"
    register: testout
  - name: dump test output
    debug:
      msg: "{{ testout.data }}"

@syncpak
Copy link
Contributor Author

syncpak commented Aug 10, 2018

@mattsene Please email service@wti.com for the user module, we can't post here until the cpm_status module is approved, since time is tight, we have a few people testing it outside this scope, just to test our basic premise. Please note the experimental user module could and WILL change at any time.

@syncpak
Copy link
Contributor Author

syncpak commented Aug 10, 2018

@vientiane Please email service@wti.com for your experimental user module questions until we post it on the github site. The answer to your question: the minimum length is 5 characters with a maximum length of 32. All WTI Ansible modules will use our RESTful API which is documented here

@syncpak
Copy link
Contributor Author

syncpak commented Aug 14, 2018

Hi @samdoran , hope your time off was awesome. When you get caught up can you peek at my comments about your change request about the common options. Thanks Ken

@samdoran
Copy link
Contributor

@syncpak The latest version of this lookup works nicely. The output it returns is looks correct now.

If i put the URL, etc in ansible.cfg would i be able to enter multiple IP addresses?

No, that wouldn't be appropriate. If you set cpm_url, etc. in ansible.cfg, that would be overridden by parameters passed to the lookup in the playbook. In order to have the lookup query different devices with different credentials using group/host_vars, you would pass the parameters directly in to the playbook.

The docs section needs to be updated to add any ini and env settings, if desired.

EXAMPLES = """
# Get temperature
- name: run Get Device Temperature
debug: msg="{{ lookup('cpm_status',
Copy link
Contributor

@samdoran samdoran Aug 14, 2018

Choose a reason for hiding this comment

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

Can you change the examples to use YAML rather than k=v shorthand?

- debug:
    var: lookup('cpm_status',
             'temperature',
             validate_certs=true,
             use_https=true,
             cpm_url='rest.wti.com',
             cpm_username='rest',
             cpm_password='restfulpassword')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @samdoran , absolutely, could you point us to an lookup example that we could pattern this after ? I am having a little trouble

Copy link
Contributor

Choose a reason for hiding this comment

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

The example I gave is a pretty good. I don't think we have any examples of using the lookup across multiple lines. There are quite a few examples that need to me updated to use YAML rather than k=v, which is confusing.

You can look at consul_kv.py or onepassword.py for examples.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 14, 2018
@syncpak
Copy link
Contributor Author

syncpak commented Aug 14, 2018

Thanks @samdoran i believe i was way over thinking things. New Examples checked in.

@dmobergIT
Copy link

Can the Temperature return be switched from Fahrenheit to Celsius

@syncpak
Copy link
Contributor Author

syncpak commented Aug 15, 2018

@dmobergIT that can be defined in the box on the system level. Via the Ansible lookup module, only one or the other will be displayed, depending on the setting in the WTI box itself.

@syncpak
Copy link
Contributor Author

syncpak commented Aug 18, 2018

Hi @samdoran, are we good ? Thanks

@samdoran
Copy link
Contributor

@syncpak Yup, looks good. Did you want to update the documentation to leverage environment variables and/or config file settings?

@syncpak
Copy link
Contributor Author

syncpak commented Aug 20, 2018

@samdoran , I think we are good. All our parameters (unfortunately or fortunately depending on who you talk to) are per a unit basis, nothing globally can be assumed. Our parameters cpm_url, cpm_username, cpm_password, use_https, validate_certs can't be globally set because usually our customers have varying uses and configurations.

from ansible.plugins.lookup import LookupBase
from ansible.module_utils._text import to_text, to_bytes
from ansible.module_utils.six.moves.urllib.error import HTTPError, URLError
from ansible.module_utils._text import to_text, to_native
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine this import with line 114.

…vious ansible.module_utils._text import line
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Aug 20, 2018
@syncpak
Copy link
Contributor Author

syncpak commented Aug 21, 2018

@samdoran i made the changes but an getting goofy errors when github is doing the checks. The First time i was getting an "unstable" warning, now i am getting some docker errors. I only made that change merging redundant lines and the code still runs when i do my check against it.

@samdoran
Copy link
Contributor

@syncpak That is "trouble in the cloud" as they say. The failures are unrelated to your changes. I re-ran the one remaining failing test to see if it passes.

@samdoran
Copy link
Contributor

Integration test failures are unrelated to this change. Merging.

@samdoran samdoran merged commit 5b78b10 into ansible:devel Aug 21, 2018
@syncpak
Copy link
Contributor Author

syncpak commented Aug 21, 2018

@sandoran thanks, i was getting paranoid.

@syncpak syncpak deleted the WTIDevice branch August 21, 2018 17:38
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_plugin This PR includes a new plugin. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants