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

cp_gaia_physical_interface initial #61370

Open
wants to merge 4 commits into
base: devel
from

Conversation

@chkp-yuvalfe
Copy link

commented Aug 27, 2019

SUMMARY

Check Point GAIA physical-interface module

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

cp_gaia_physical_interface
cp_gaia_physical_interfaces_facts

ADDITIONAL INFORMATION

In addition to the modules:

  1. Added function to module_utils/network/checkpoint/checkpoint.py of calling gaia_api APIs
  2. Changed httpapi/checkpoint.py plugin to get CONTEXT environment variable if exist. If not exist, set the url to the original one (web_api). I do that in order to get 'gaia_api' context.
@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@chkp-yuvalfe this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@chkp-yuvalfe, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/module_utils/network/checkpoint/checkpoint.py:445:0: trailing-whitespace Trailing whitespace
test/units/plugins/httpapi/test_checkpoint.py:13:0: syntax-error Cannot import 'ansible.plugins.httpapi.checkpoint' due to syntax error 'invalid syntax (<unknown>, line 44)'

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

lib/ansible/plugins/httpapi/checkpoint.py:44:0: syntax-error invalid syntax (<unknown>, line 44)

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

lib/ansible/plugins/httpapi/checkpoint.py:0:0: missing documentation (or could not parse documentation): invalid syntax (<unknown>, line 44)

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/plugins/httpapi.rst:44:0: toc-tree-glob-pattern-no-match: toctree glob pattern 'httpapi/*' didn't match any documents

The test ansible-test sanity --test package-data [explain] failed with the error:

Command "/usr/bin/python3.6 /root/ansible/test/sanity/code-smell/package-data.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 383, in <module>
    main()
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 360, in main
    sdist_path = create_sdist(tmp_dir)
  File "/root/ansible/test/sanity/code-smell/package-data.py", line 174, in create_sdist
    raise Exception('make snapshot failed:\n%s' % stderr)
Exception: make snapshot failed:
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
fatal: not a git repository (or any parent up to mount point /)
Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
docs/man/man1/ansible-galaxy.1.rst:51: (WARNING/2) Definition list ends without a blank line; unexpected unindent.
docs/man/man1/ansible-galaxy.1.rst:57: (WARNING/2) Definition list ends without a blank line; unexpected unindent.
ERROR! httpapi checkpoint at /tmp/tmpzfqmbjgh/lib/ansible/plugins/httpapi/checkpoint.py has a documentation error formatting or is missing documentation.
Traceback (most recent call last):
  File "packaging/release/changelogs/changelog.py", line 835, in <module>
    main()
  File "packaging/release/changelogs/changelog.py", line 102, in main
    args.func(args)
  File "packaging/release/changelogs/changelog.py", line 132, in command_release
    plugins = load_plugins(version=version, force_reload=reload_plugins)
  File "packaging/release/changelogs/changelog.py", line 184, in load_plugins
    '--json', '--metadata-dump', '-t', plugin_type])
  File "/usr/lib/python3.6/subprocess.py", line 356, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/tmp/tmpzfqmbjgh/bin/ansible-doc', '--json', '--metadata-dump', '-t', 'httpapi']' returned non-zero exit status 1.
make: *** [changelog] Error 1

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

The test ansible-test sanity --test compile --python 3.8 [explain] failed with 1 error:

lib/ansible/plugins/httpapi/checkpoint.py:44:36: SyntaxError: if CONTEXT == "web_api"

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

lib/ansible/module_utils/network/checkpoint/checkpoint.py:445:1: E101 indentation contains mixed spaces and tabs
lib/ansible/module_utils/network/checkpoint/checkpoint.py:445:1: W191 indentation contains tabs
lib/ansible/module_utils/network/checkpoint/checkpoint.py:445:1: W293 blank line contains whitespace
lib/ansible/module_utils/network/checkpoint/checkpoint.py:447:1: E302 expected 2 blank lines, found 1
lib/ansible/module_utils/network/checkpoint/checkpoint.py:448:1: E101 indentation contains mixed spaces and tabs
lib/ansible/plugins/httpapi/checkpoint.py:45:17: E113 unexpected indentation

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:28:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:29:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:31:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:32:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:28:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:29:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:31:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:32:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:54:3: key-duplicates EXAMPLES: duplication of key "cp_gaia_physical_interfaces_facts" in mapping
lib/ansible/plugins/httpapi/checkpoint.py:44:36: python-syntax-error invalid syntax (<unknown>, line 44)

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

The test ansible-test sanity --test pylint [explain] failed with 2 errors:

lib/ansible/plugins/httpapi/checkpoint.py:37:18: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/httpapi/checkpoint.py:52:14: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test validate-modules [explain] failed with 10 errors:

lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:28:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:29:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:31:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interface.py:32:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:0:0: E312 No RETURN provided
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:28:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:29:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:31:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:32:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.

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

lib/ansible/modules/network/checkpoint/cp_gaia_physical_interfaces_facts.py:54:3: key-duplicates EXAMPLES: duplication of key "cp_gaia_physical_interfaces_facts" in mapping

click here for bot help

cp_gaia_physical_interface:
comments: eth0 interface
enabled: true
name: eth0

This comment has been minimized.

Copy link
@justjais

justjais Aug 29, 2019

Contributor

state option is needed by Ansible module to make it aligned to Ansible conventio std., and once state option is included plz include the example for state: absent i.e. removal example as well (ref: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/checkpoint/checkpoint_host.py#L47)

This comment has been minimized.

Copy link
@chkp-yuvalfe

chkp-yuvalfe Aug 29, 2019

Author

state parameter won't be useful here - since it physical interface, you cannot add or remove it, just edit the configurations of existing interface.
I can add state parameter but I think it will just confuse users because I don't have something to do with the absent/present values.


module_params_show = dict((k, v) for k, v in module.params.items() if k in ["name"] and v is not None)

module.params = module_params_show

This comment has been minimized.

Copy link
@justjais

justjais Aug 29, 2019

Contributor

as I see the implementation here, I am not sure why u are calling the api in sequence of show->set->show, Ideally for the respective module only set-api should get called and it's respective response should be displayed to the user. So, you'll need to remove the calls to show-api.

This comment has been minimized.

Copy link
@chkp-yuvalfe

chkp-yuvalfe Aug 29, 2019

Author

Only set-api is needed, but we need the show-api in order to achieve idem-potency. when we call just the set-api we don't know if something really changed, so we call the show-api before and after to check if there were changes following the set-api.
Tell me if you think it's redundant and it's ok to return always the same value (true/false) on was_changed.

@@ -33,20 +33,23 @@ class HttpApi(HttpApiBase):
def login(self, username, password):
if username and password:
payload = {'user': username, 'password': password}
url = '/web_api/login'
CONTEXT = os.environ.get('CONTEXT', 'web_api')

This comment has been minimized.

Copy link
@justjais

justjais Aug 29, 2019

Contributor

may I know the reason of this change

This comment has been minimized.

Copy link
@chkp-yuvalfe

chkp-yuvalfe Aug 29, 2019

Author

We use the same connection (httpapi), but for gaia APIs the url should be 'gaia_api' instead of 'web_api'. So I didn't find better solution than set CONTEXT environment variable that will have the value 'gaia_api' when we want to use it for gaia modules.
If we don't have it, the value will stay 'web_api'.

Tell me if there is better solution - like passing it as a parameter from the module code. Thanks.

@ansibot ansibot removed the needs_triage label Aug 29, 2019

@ansibot ansibot added the stale_ci label Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.