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

Cpm serial port config #55102

Open
wants to merge 18 commits into
base: devel
from

Conversation

Projects
None yet
9 participants
@syncpak
Copy link
Contributor

syncpak commented Apr 10, 2019

SUMMARY

Added the module cpm_serial_port_config to be able configure various serial port parameters of WTI OOB and PDU devices.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

cpm_serial_port_config

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 10, 2019

@syncpak this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@syncpak syncpak force-pushed the syncpak:cpm_serial_port_config branch from 8bad611 to edca789 Apr 10, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 10, 2019

@wtinetworkgear

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

@scarshouldofwon

This comment has been minimized.

Copy link

scarshouldofwon commented Apr 10, 2019

Does this also work with the DSM line of boxes?

@wtinetworkgear

This comment has been minimized.

Copy link

wtinetworkgear commented Apr 10, 2019

@scarshouldofwon ,yes this module will work on the entire line of WTI OOB and PDU devices, including the DSM. You don’t need the latest firmware, but something within the past two years would be nice. Although updating to the latest firmware is always encouraged.

@dmobergIT

This comment has been minimized.

Copy link

dmobergIT commented Apr 10, 2019

In your DOCUMENTATION section, there is still reference to "cpm_portconfig" as your module name, not the new "cpm_serial_port_config"

@syncpak

This comment has been minimized.

Copy link
Contributor Author

syncpak commented Apr 10, 2019

Thanks @dmobergIT , fixed and checked in

@mattsene

This comment has been minimized.

Copy link

mattsene commented Apr 11, 2019

Do you have any documentation defining the output JSON of the module?

@samdoran

This comment has been minimized.

Copy link
Member

samdoran commented Apr 11, 2019

I'm not sure about those test failures. I'll see if I can figure out what's up there.

@samdoran

This comment has been minimized.

Copy link
Member

samdoran commented Apr 11, 2019

The test failure is unrelated to the this PR. We will have to fix it in devel.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Apr 11, 2019

@syncpak this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansible ansible deleted a comment from ansibot Apr 11, 2019

@ansible ansible deleted a comment from ansibot Apr 11, 2019

@samdoran

This comment has been minimized.

Copy link
Member

samdoran commented Apr 11, 2019

We narrowed down the cause of the error and merged a fix. I'll re-run the failed tests.

@samdoran
Copy link
Member

samdoran left a comment

Works well overall. Just a few changes to the check mode behavior and a few UI changes to params.

Show resolved Hide resolved lib/ansible/modules/remote_management/cpm/cpm_serial_port_config.py
Show resolved Hide resolved lib/ansible/modules/remote_management/cpm/cpm_serial_port_config.py
total_change = (total_change | 512)
json_load = '%s,"echo": %s' % (json_load, to_native(cpmmodule.params["echo"]))
if cpmmodule.params["break_allow"] is not None:
if (existing_serial["serialports"][0]["break"] != to_native(cpmmodule.params["break_allow"])):

This comment has been minimized.

Copy link
@samdoran

samdoran Apr 11, 2019

Member

If you change this param to be type bool, you'll need no convert these with int() when comparing. Since existing_serial["serialports"][0]["break"] is a text string and cpmmodule.params["break_allow"] would be a bool.

@samdoran

This comment has been minimized.

Copy link
Member

samdoran commented Apr 11, 2019

We're still working on fixing the test error. More tests are being triggered because of the change in config/module_defaults.yml. Sorry for the headache around that. It's a nice feature to have though. We should probably put a note or something in the modules that you can use groups/cpm: in module_defaults. It's a relatively new feature documented here.

Could you go ahead and squash away the merge commit in the mean time? I don't care if you preserve my info in the history so long as you get the changes in.

If you want to kick tests, you can create an empty commit with just a message by running git commit -m 'Message' --allow-empty. Closing and reopening works as well.

wtinetworkgear added some commits Apr 11, 2019

converted echo and break_allow parameter to bool
fixed change logic in check mode
fixed stopbit description
@syncpak

This comment has been minimized.

Copy link
Contributor Author

syncpak commented Apr 11, 2019

Hi @samdoran No worries, I checked in new code (the checks failed but i am pretty sure all looks well) with the changes requested, except for converting the "cmd" parameter to bool, that caused an internal company uproar of what the "cmd" parameter does, so i let sleeping dogs lie.

@syncpak

This comment has been minimized.

Copy link
Contributor Author

syncpak commented Apr 15, 2019

shipit

1 similar comment
@wtinetworkgear

This comment has been minimized.

Copy link

wtinetworkgear commented Apr 15, 2019

shipit

@wtinetworkgear

This comment has been minimized.

Copy link

wtinetworkgear commented Apr 16, 2019

Hi @samdoran , are we too late getting this module and the "Cpm serial port config #55102" in the 2.8 build ? If there is anything we need to do just let us know. Thanks

@jillr jillr removed the needs_triage label Apr 18, 2019

@samdoran

This comment has been minimized.

Copy link
Member

samdoran commented Apr 22, 2019

@wtinetworkgear Yes, this is too late for 2.8.

@ansibot ansibot added the stale_ci label Apr 22, 2019

@samdoran samdoran self-requested a review Apr 22, 2019

@samdoran
Copy link
Member

samdoran left a comment

Just a few more minor things.

DOCUMENTATION = """
---
module: cpm_serial_port_config
version_added: "2.8"

This comment has been minimized.

Copy link
@samdoran

samdoran Apr 22, 2019

Member

Needs to be 2.9 now.

cmd=dict(type='int', required=False, default=None, choices=[0, 1]),
seq=dict(type='int', required=False, default=None, choices=[1, 2, 3]),
tout=dict(type='int', required=False, default=None, choices=[0, 1, 2, 3, 4, 5]),
echo=dict(type='bool', required=False, default=None, choices=[0, 1]),

This comment has been minimized.

Copy link
@samdoran

samdoran Apr 22, 2019

Member

choices isn't really needed for type='bool'.

seq=dict(type='int', required=False, default=None, choices=[1, 2, 3]),
tout=dict(type='int', required=False, default=None, choices=[0, 1, 2, 3, 4, 5]),
echo=dict(type='bool', required=False, default=None, choices=[0, 1]),
break_allow=dict(type='bool', required=False, default=None, choices=[0, 1]),

This comment has been minimized.

Copy link
@samdoran

samdoran Apr 22, 2019

Member

Same here, no need for choices.

description:
- This is the logout character to assign to the port
- If preceded by a ^ character, the sequence will be a control character. Used if seq is set to 0 or 1
required: false

This comment has been minimized.

Copy link
@samdoran

samdoran Apr 22, 2019

Member

Add a note about using module_defaults:

Suggested change
required: false
required: false
notes:
- Use C(groups/cpm) in C(module_defaults) to set common options used between CPM modules.
bumped up version_added
removed unneeded boolean choice tags

@ansibot ansibot removed the stale_ci label Apr 23, 2019

wtinetworkgear added some commits Apr 23, 2019

@syncpak

This comment has been minimized.

Copy link
Contributor Author

syncpak commented Apr 23, 2019

bumped up the version_added and removed the bool choices, thanks

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.