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

network and install policy modules for example #55446

Open
wants to merge 18 commits into
base: devel
from

Conversation

@chkp-orso
Copy link

commented Apr 17, 2019

network and install policy modules for example

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

network and install policy modules for example
network and install policy modules for example
@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@chkp-orso 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 Apr 17, 2019

@chkp-orso, 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

revert the changes in version
revert the changes in version

@justjais justjais self-requested a review Apr 18, 2019

@@ -63,7 +63,7 @@ def send_request(self, path, body_params):

return response.getcode(), self._response_to_json(value)
except AnsibleConnectionFailure as e:
return 404, 'Object not found'

This comment has been minimized.

Copy link
@justjais

justjais Apr 18, 2019

Contributor

have u tested this exception case, and is it working as expected?

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso Apr 18, 2019

Author

@justjais I tested it, but it's not working. I don't get the complete error of checkpoint..

@ansibot ansibot removed the ci_verified label Apr 18, 2019

chkp-orso added some commits Apr 22, 2019

documentation_fragment
documentation_fragment

@mattclay mattclay added the ci_verified label May 2, 2019

@ansibot ansibot removed the ci_verified label May 2, 2019

chkp-orso added some commits May 5, 2019

@justjais
Copy link
Contributor

left a comment

@chkp-orso First off thanks for all your efforts in contributing these CP modules, and there are few cosmetic n minor duplicate comments in all the new module that u have pushed. Plz update the module as per the comment in all the PR modules.

@@ -52,10 +52,10 @@
"""

RETURN = """
ansible_facts:
description: The checkpoint access layer facts.
api_result:

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

@chkp-orso why have u modified return param from ansible_facts to api_result

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 13, 2019

Author

@justjais because I wanted it to be consistent, means all the files return api_result.. (all the modules and all the commands like install policy return api_result).
Isn't it better?

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

@chkp-orso to be consistent with other Ansible resource module, it's better to have as is. Also, it's better to have each module returns the module facts instead of common return.

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais So to write "ansible_facts" or "checkpoint_access_layer_facts" ?

This comment has been minimized.

Copy link
@justjais

justjais May 15, 2019

Contributor

@chkp-orso for already existing module, u need not to change the return values, respective comment was for future modules

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais amm but I would prefer all the modules to be consistent..
because I create all the modules automatically with the tool..

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

So I would prefer to return "ansible_facts" or "checkpoint_access_layer_facts" to all files

This comment has been minimized.

Copy link
@justjais

justjais May 16, 2019

Contributor

@chkp-orso It's better to return the facts with the module name, but if not u can return with ansible_facts for e.g. for module checkpoint_host the return value is checkpoint_hosts but for module checkpoint_access_layer_facts the return value is ansible_facts

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 16, 2019

Author

@justjais OK, so anyway because we prefer to be consistent, we will return "ansible_facts" to all the modules

description: The checkpoint access rule object created or updated.
returned: always, except when deleting the access rule.
type: list
api_result:

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

Return value is the value which is being returned as result to user and in this case result['checkpoint_access_rules'] is being returned as output to user, so u need not not modify the return if the param name itself is not changed.

@@ -59,10 +59,10 @@
"""

RETURN = """
ansible_facts:
description: The checkpoint access rule object facts.
api_result:

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

same as previous module return comment.

mutually_exclusive=[['name', 'uid']])
api_call_object = "address-range"

api_call(module, api_call_object)

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

@chkp-orso plz update the api_call method to return the result as output and handling of the result should be part of the module code, it's the std. way of all the ansible resource module.

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor
Suggested change
api_call(module, api_call_object)
result = api_call(module, api_call_object)
result['checkpoint_address_range'] = response
module.exit_json(**result)

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais
Is it OK to write like this:

result = api_call(module, api_call_object)
module.exit_json(**result)

This comment has been minimized.

Copy link
@justjais

justjais May 15, 2019

Contributor

@chkp-orso yes, u can keep that way.

This comment has been minimized.

Copy link
@justjais

justjais May 16, 2019

Contributor

@chkp-orso as per your comment of using the ansible_facts as return from each module to be consistent, you'll need to modify this logic as otherwise this shall return the module api passed as response instead of ansible_facts

api_call_object = "address-range"
api_call_object_plural_version = "address-ranges"

api_call_facts(module, api_call_object, api_call_object_plural_version)

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

same as api_call result as output comment.

"""

RETURN = """
api_result:

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor
Suggested change
api_result:
checkpoint_network:
mutually_exclusive=[['name', 'uid']])
api_call_object = "network"

api_call(module, api_call_object)

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

same as previous comment.


ANSIBLE_METADATA = {'metadata_version': '1.1',
'status': ['preview'],
'supported_by': 'network'}

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor
Suggested change
'supported_by': 'network'}
'supported_by': 'community'}

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais Are u sure about that?
that's how Riki wrote in all modules..

This comment has been minimized.

Copy link
@chkp-orso

This comment has been minimized.

Copy link
@justjais

justjais May 15, 2019

Contributor

@chkp-orso yes, since Ricky was part of Anisble networking and he wrote the modules, so it'll be supported by network and since the new CP modules shall be contributed by u it shall be community

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais But as Riki once told us, he just wrote a quick example, the modules he wrote are not good enough from checkpoint aspect, so anyway I would prefer to delete and rewrite all his modules..
Is it possible?

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

@justjais So just to make sure I understand: it's not possible to delete and rewrite Rikis' modules in version 2.9?
only thing possible - to apply changes?

This comment has been minimized.

Copy link
@cross-logic

cross-logic May 15, 2019

@chkp-orso @justjais you can always write new modules to replace them and then deprecate the old ones in 2.9. Before doing that, I would recommend to evaluate if you can achieve what you are looking for through updates. That would probably be a better experience from a user standpoint.

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 15, 2019

Author

Thanks!, nevertheless I would prefer replace those files, because they are totally different.
So how can I upload those new files?

This comment has been minimized.

Copy link
@justjais

justjais May 16, 2019

Contributor

@cross-logic @chkp-orso If u want to re-write the module post rigorous evaluation that the respective module cannot serve ur purpose then in that case you would need to make sure the changes are backwards compatible, e.g. don't remove module options or change the return values without a deprecation period.

This comment has been minimized.

Copy link
@chkp-orso

chkp-orso May 16, 2019

Author

@justjais great thanks!

"""

RETURN = """
api_result:

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor
Suggested change
api_result:
checkpoint_network_facts:
api_call_object = "network"
api_call_object_plural_version = "networks"

api_call_facts(module, api_call_object, api_call_object_plural_version)

This comment has been minimized.

Copy link
@justjais

justjais May 13, 2019

Contributor

same as previous comment.

@ikhan2010 ikhan2010 requested a review from maxamillion May 20, 2019

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.