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 vyos_interfaces resource module [vyos_interfaces] #58589

Open
wants to merge 7 commits into
base: devel
from

Conversation

@NilashishC
Copy link
Contributor

commented Jul 1, 2019

Signed-off-by: NilashishC nilashishchakraborty8@gmail.com

SUMMARY
ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vyos_interfaces.py

@ansibot

This comment has been minimized.

@ansibot

This comment was marked as off-topic.

Copy link
Contributor

commented Jul 1, 2019

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

lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:17:0: trailing-whitespace Trailing whitespace
lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:18:11: bad-whitespace Exactly one space required around assignment     choices=[            ^

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

lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:17:1: W293 blank line contains whitespace
lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:18:12: E225 missing whitespace around operator

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

lib/ansible/module_utils/network/vyos/utils/utils.py:0:0: should not have a shebang

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

lib/ansible/modules/network/vyos/vyos_facts.py:0:0: E307 version_added should be '2.2'. Currently 2.9
lib/ansible/modules/network/vyos/vyos_facts.py:0:0: E326 Argument 'gather_network_resources' in argument_spec defines choices as ([['all'], ['interfaces']]) but documentation defines choices as ([])
lib/ansible/modules/network/vyos/vyos_facts.py:74:9: E311 EXAMPLES is not valid YAML
lib/ansible/modules/network/vyos/vyos_interfaces.py:0:0: E305 DOCUMENTATION.options.config.elements: extra keys not allowed @ data['options']['config']['elements']. Got 'dict'
lib/ansible/modules/network/vyos/vyos_interfaces.py:0:0: E305 DOCUMENTATION.options.config.suboptions.vifs.elements: extra keys not allowed @ data['options']['config']['suboptions']['vifs']['elements']. Got 'dict'
lib/ansible/modules/network/vyos/vyos_interfaces.py:0:0: E337 Argument 'config' in argument_spec defines type as 'list' but documentation doesn't define type

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Jul 1, 2019

@ansibot

This comment was marked as off-topic.

Copy link
Contributor

commented Jul 1, 2019

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

lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:15:0: trailing-whitespace Trailing whitespace
lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:16:11: bad-whitespace Exactly one space required around assignment     choices=[            ^

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

lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:15:1: W293 blank line contains whitespace
lib/ansible/module_utils/network/vyos/argspec/facts/facts.py:16:12: E225 missing whitespace around operator

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

lib/ansible/module_utils/network/vyos/utils/utils.py:0:0: should not have a shebang

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

lib/ansible/modules/network/vyos/vyos_facts.py:0:0: E307 version_added should be '2.2'. Currently 2.9
lib/ansible/modules/network/vyos/vyos_facts.py:0:0: E326 Argument 'gather_network_resources' in argument_spec defines choices as ([['all'], ['interfaces']]) but documentation defines choices as ([])
lib/ansible/modules/network/vyos/vyos_facts.py:74:9: E311 EXAMPLES is not valid YAML
lib/ansible/modules/network/vyos/vyos_interfaces.py:0:0: E305 DOCUMENTATION.options.config.suboptions.vifs.elements: extra keys not allowed @ data['options']['config']['suboptions']['vifs']['elements']. Got 'dict'

click here for bot help

@ansibot ansibot added the ci_verified label Jul 1, 2019

@NilashishC NilashishC force-pushed the NilashishC:vyos_interfaces branch from 35419e1 to 18b3c16 Jul 1, 2019

@ansibot ansibot removed the ci_verified label Jul 1, 2019

@NilashishC NilashishC force-pushed the NilashishC:vyos_interfaces branch from 18b3c16 to 87033c8 Jul 1, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jul 1, 2019

@ansible-zuul

This comment was marked as outdated.

Copy link

commented Jul 1, 2019

Build failed (third-party-check pipeline) integration testing with
Ansible.

@pabelanger
Copy link
Contributor

left a comment

-1 here to ensure we do not merge this code until we discuss the testing strategy. My understanding so far, is our new resource modules require 1.2.x of vyos, but currently we only have 1.1.8 images available. We can upgrade our vyos image in zuul, but we should confirm if that is something we really want to do.

Currently 1.1.8 is the legacy images, which do not appear to be supported anymore upstream, but assume that users still use them. By switching to 1.2.x, we get the latest images, but due to new license module we cannot run released versions, but we have the ability to use rolling releases (latest builds).

What we need to decided, by switching to 1.2.x, are we also going to continue running 1.1.8 against devel (target for ansible 2.9) or run both images? By upgrading to 1.2.x, we also need to ensure that all our existing code works with 1.2.x, and deal with any functional changes too.

We would continue to run 1.1.8 on stable-2.8, it would not have the ability to run 1.2.x.

@ansible-zuul

This comment has been minimized.

Copy link

commented Jul 5, 2019

Build succeeded (third-party-check pipeline).

@ansibot ansibot added needs_revision and removed core_review labels Jul 5, 2019

@ikhan2010 ikhan2010 added this to Needs Triage in Networking via automation Jul 8, 2019

@ikhan2010 ikhan2010 moved this from Needs Triage to 2.9 MUST in Networking Jul 8, 2019

@ikhan2010 ikhan2010 moved this from 2.9 MUST to Review in Networking Jul 8, 2019

@ikhan2010 ikhan2010 changed the title Add vyos_interfaces resource module Add vyos_interfaces resource module [vyos_interfaces] Jul 8, 2019

@ikhan2010 ikhan2010 added this to Modules in Development in Ansible 2.9 Networking Jul 8, 2019

@ikhan2010 ikhan2010 moved this from Modules in Development to Modules in Review in Ansible 2.9 Networking Jul 8, 2019

@ikhan2010 ikhan2010 removed this from Review in Networking Jul 15, 2019

@ikhan2010 ikhan2010 removed this from Modules in Review in Ansible 2.9 Networking Jul 15, 2019

@ikhan2010 ikhan2010 added this to Module in Review in Ansible 2.9 Networking Feature Dev Jul 15, 2019

@ikhan2010

This comment has been minimized.

Copy link

commented Jul 16, 2019

@pabelanger I believe we agreed on the testing strategy. Do you have any other reservations?

@ikhan2010

This comment has been minimized.

Copy link

commented Jul 16, 2019

@justjais @ganeshrn - Please review the code, so we can merge it.

@pabelanger
Copy link
Contributor

left a comment

Nope, testing is working now, on vyos 1.1.8. We still need to bring 1.2.x online, still working to do that.

# GNU General Public License v3.0+
# (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
"""
The junos interfaces fact class

This comment has been minimized.

Copy link
@rohitthakur2590

rohitthakur2590 Jul 17, 2019

Contributor

I think it should be VyOS

:rtype: dictionary
:returns: facts
"""
if connection: # just for linting purposes, remove

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

If this is not required it can be removed

# utils


def search_obj_in_list(name, lst, key='name'):

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

If this function is used across platforms for resource modules can it be moved to network/common/utils

respective resource name. The facts module will always collect a
base set of facts from the device and can enable or disable
collection of additional facts.
author: Nilashish Chakraborty (@nilashishc)

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

you can add multiple authors as a list here :-)

return commands

@staticmethod
def _state_replaced(**kwargs):

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

What is the reason to use kwargs here? Since the argument list is defined I don't see a reason to use kwargs. Applicable at other places as well in this file

@ganeshrn
Copy link
Member

left a comment

Please deprecate the existing vyos_interface module.

want_intf = kwargs['want_intf']
have_intf = kwargs['have_intf']

if have_intf:

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

Since replaced is equivalent to deleted of existing attributes in want and after that merged, can this be refactored to reuse the state=deleted logic
Something similar to

commands.extend(Interfaces._state_deleted(want, have))
commands.extend(Interfaces._state_merged(want, have))
want_intfs = kwargs['want']
have_intfs = kwargs['have']

for have_intf in have_intfs:

This comment has been minimized.

Copy link
@ganeshrn

ganeshrn Jul 17, 2019

Member

Similar comment as above.

commands.extend(Interfaces._state_replaced(want, have))
commands.extend(Interfaces._state_deleted(<objs in have but not in want>, have))
for item in want:
name = item['name']
obj_in_have = search_obj_in_list(name, have)
if state == 'deleted':

This comment has been minimized.

Copy link
@rohitthakur2590

rohitthakur2590 Jul 18, 2019

Contributor

aren't we going to handle the scenario where playbook won't have any interface mentioned and just the
state: deleted which will lead to deleting the attributes of all the interfaces which were configured by vyos_interfaces
RM.

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.