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

adding name option for textfsm to create facts to key #202

Merged
merged 2 commits into from
Dec 3, 2018

Conversation

jwoogee
Copy link
Contributor

@jwoogee jwoogee commented Nov 23, 2018

Adding name option for textfsm so it has a key to set ansible_facts to. This is related to changes to ansible-network.cisco_ios, found here, ansible-network/cisco_ios#51

Signed-off-by: jwoogee jswoods1@gmail.com

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@@ -98,6 +98,7 @@ def run(self, tmp=None, task_vars=None):
command = self._task.args['command']
parser = self._task.args.get('parser')
engine = self._task.args.get('engine', 'command_parser')
key_name = self._task.args.get('key_name')
Copy link
Member

Choose a reason for hiding this comment

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

Instead of key_name can you please keep the term same as name.
Also please add documentation for the new option here

DOCUMENTATION = """
---
module: cli
author: Ansible Network Team
short_description: Runs the specific command and returns the output
description:
- The command specified in C(command) will be executed on the remote
device and its output will be returned to the module. This module
requires that the device is supported using the C(network_cli)
connection plugin and has a valid C(cliconf) plugin to work correctly.
version_added: "2.5"
options:
command:
description:
- The command to be executed on the remote node. The value for this
argument will be passed unchanged to the network device and the
output returned.
required: yes
default: null
parser:
description:
- The parser file to pass the output from the command through to
generate Ansible facts. If this argument is specified, the output
from the command will be parsed based on the rules in the
specified parser.
default: null
engine:
description:
- Defines the engine to use when parsing the output. This argument
accepts one of two valid values, C(command_parser) or C(textfsm_parser).
default: command_parser
choices:
- command_parser
- textfsm_parser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking into this more, should we call it register to match what we typically do when assigning keys in ansible? Let me know if you have a preference and I'll update it.

Copy link
Member

@trishnaguha trishnaguha Nov 27, 2018

Choose a reason for hiding this comment

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

@jwoogee textfsm_parser module uses the parameter name to return ansible_facts https://github.com/ansible-network/network-engine/blob/devel/library/textfsm_parser.py#L46.
We don't want to call it register as it might create confusion in case there is a necessity of using register from ansible in the same task.

Copy link
Contributor Author

@jwoogee jwoogee Nov 30, 2018

Choose a reason for hiding this comment

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

Thank you @trishnaguha , I've changed the term to 'name'.
Would you be able to help with PR ansible-network/cisco_ios#51 or do you know who I can assign as reviewer on that one?

Copy link
Member

@trishnaguha trishnaguha Dec 3, 2018

Choose a reason for hiding this comment

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

Yes I can definitely help with that one.

@trishnaguha trishnaguha self-assigned this Nov 26, 2018
jwoogee and others added 2 commits November 30, 2018 14:17
Signed-off-by: jwoogee <jswoods1@gmail.com>
Signed-off-by: z003gn3 <jonathon.woods@target.com>
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants