search

ansible / proposals

Repository for sharing and tracking progress on enhancement proposals for Ansible.
Creative Commons Zero v1.0 Universal
93 stars 19 forks source link

Refactor common network shared code into package #76

Closed privateip closed 6 years ago

privateip commented 6 years ago

Date: 2017/9/21

Motivation

The rapid pace of module development for network modules has amassed a significant amount of technical debt related to the network shared code found in ansible/module_utils. This proposal is designed to provide details on creating a stable network package in ansible/module_utils/network and move the current shared network libraries into the new package. The ansible.module_utils.network package will only contain shared code that is common for all network modules to implement and will be maintained in a stable way moving forward.

[edit] Amending this proposal to include a note of clarification. The only code that will live in the newly proposed package would be things that are truly platform agnostic and shareable across any and all network modules. This means there should be no platform specific code in this package

Problems

What problems exist that this proposal will solve?

What problems exist that this proposal will solve?

Solution proposal

In order to solve the problems mentioned above, this proposal will refactor a number of shared functions that currently reside in module_utils. The goal for the refactoring will be to introduce changes only where necessary to complete the refactor. All of the current functions in module_utils will remain and be deprecated per community process so as to provide backwards compatibility.

In order to solve the current problem, the following is proposed:

Performing this work will help to stabilize the common code base used for developing network modules and add the missing test cases. Once completed new changes introduced into ansible.module_utils.network will be done to maintain backwards compatibility, keep the package stable and well documented.

Dependencies (optional)

Testing (optional)

See comments in proposal around testing

Documentation (optional)

See comments in proposal around documentation

Anything else?

dagwieers commented 6 years ago

Looking good. I assume that you mean

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py ?

gundalow commented 6 years ago

@dagwieers

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py?

Answer: No. The new new directory is for code shared by multiple platforms

privateip commented 6 years ago

@dagwieers

Looking good. I assume that you mean Refactor module_utils/netcfg.py into module_utils/network/config.py Refactor module_utils/network_common.py into module_utils/network/common.py

Thanks, corrected in the proposal

Do we expect that e.g. ACI modules will also move their common library from module_utils/aci.py to module_utils/network/aci.py ?

Only if there are things that are truly common across anything network related. For instance, one thing that might common is functions for manipulating IP address information.

gundalow commented 6 years ago

+1 to proposal

1) Does 2 weeks include time to write unit tests? Current coverage for the libraries is fairly low, so we will need to write a lot of new unit tests https://codecov.io/gh/ansible/ansible/tree/devel/lib/ansible/module_utils

2) Is this something we plan to do early during 2.5 development to allow partners to update their code to use these newer stable functions?

3) Is there any non-network specific code that is currently in these files that need to be moved else where (not basic.py), thinking about common filters

privateip commented 6 years ago

I have updated the proposal to include a comment about what type of code should reside in ansible.module_utils.network.

[edit] Amending this proposal to include a note of clarification. The only code that will live in the newly proposed package would be things that are truly platform agnostic and shareable across any and all network modules. This means there should be no platform specific code in this package

privateip commented 6 years ago

Does 2 weeks include time to write unit tests? Current coverage for the libraries is fairly low, so we will need to write a lot of new unit tests https://codecov.io/gh/ansible/ansible/tree/devel/lib/ansible/module_utils

Yes

Is this something we plan to do early during 2.5 development to allow partners to update their code to use these newer stable functions?

Yes, I don't anticipate much push back in getting this approved so the implementation can start very early.

Is there any non-network specific code that is currently in these files that need to be moved else where (not basic.py), thinking about common filters

Its a fair ask. I will go investigate and respond here

gundalow commented 6 years ago

Will also need documenting in https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst (create developers section under networking)

privateip commented 6 years ago

Will also need documenting in https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst (create developers section under networking)

Proposal updated with comment

abadger commented 6 years ago

Code moved from plugins/filters/network.py will need to be relicensed.

privateip commented 6 years ago

[edit] relicense code from filter to module_utils moving from GPL to BSD

Proposal updated with comment about re-licensing

itdependsnetworks commented 6 years ago

This seems like a good long term move, but in terms of priorities where does this fit? To me there are more pressing issues.

dagwieers commented 6 years ago

Ken, let's collect a list of stuff that people think is important on the Wiki at: https://github.com/ansible/community/wiki/Network:Core-community-plan

Having an overview of what people think is needed will help in structuring possible areas of attention and is helpful even for new or existing work. For the Windows Working Group this really helped solve some misconceptions, find areas that need improvement (docs, community), and bring in new ideas.

That said, people are always free to work on what they like or need themselves. It helps if we have a consensus of what we want, and how to get there. And the more people joining that conversation, the better the result.

dagwieers commented 6 years ago

For Windows we started off using Etherpad, which is very good for collaborating on content, but very bad for synthesizing that information. So we ended up moving everything into the Wiki. Maybe we could do the same for the Network WG ?

The Github Wiki is very (very, very) bad at concurrent access so probably Etherpad is best to start from.

itdependsnetworks commented 6 years ago

Agreed, I think @gundalow was collecting all of our initial comments. (Correct me if I am wrong)

dagwieers commented 6 years ago

I thought that was more about how the Network WG could be improved.

gundalow commented 6 years ago

@itdependsnetworks I was mainly collecting feedback around the Working Group process, no about specific development tasks.

privateip commented 6 years ago

approved to be implemented in ansible networking irc meeting

dagwieers commented 6 years ago

So, there's an argument wrt. if module_utils/network/ is to be used for all network-related libraries or only the generic libraries. Since the name is quite generic "network" and we have a lot of vendor-specific libraries in module_utils/ it is probably wise to have a similar structure in module_utils/ as we already have established in modules/ which means avi.py would live in module_utils/network/.

privateip commented 6 years ago

Holding this for one more week before finalizing to address @dagwieers comments. Original proposal is to create a package that does not include platform specific code but only common shared code for network modules.

Idea to change the package name to one of the following:

Amendment on the table to create a structure that more reflects the modules/ layout in module_utils/. Discussion to be added to core irc meeting agenda and hashed out there

privateip commented 6 years ago

After having some one off individual discussions, proposal has been updated to move forward with the following packaging:

dagwieers commented 6 years ago

@privateip Perfect as that will work much better with how ansibot is designed. With this scheme it will automatic label PRs with files in these directories.

gundalow commented 6 years ago

AGREED in Network IRC meeting Wednesday 4th Oct

caphrim007 commented 6 years ago

how does the proposal accommodate cases where the following occurs.

bigip* and bigiq* share code amongst themselves (classes, functions, constants). One does not want to copy the module code from one to the other

Would any of the following be true? (examples are directories unless otherwise specified). Which of the following would be preferred

Case 1 ansible.module_utils.network.bigiq (specific to bigiq) ansible.module_utils.network.bigip (specific to bigip) ansible.module_utils.network.f5 (common for all F5 products)

Case 2 ansible.module_utils.network.f5.bigiq (specific to bigiq) ansible.module_utils.network.f5.bigip (specific to bigip) ansible.module_utils.network.f5.common (common for all F5 products)

Case 3 ansible.module_utils.network.bigiq (specific to bigiq) ansible.module_utils.network.bigip (specific to bigip) ansible.module_utils.network.f5.common (a file, common for all F5 products)

Case 4 ansible.module_utils.network.bigiq (specific to bigiq) ansible.module_utils.network.bigip (specific to bigip) ansible.module_utils.network.f5_utils (a file, common for all F5 products)

privateip commented 6 years ago

Case 2 would be the preferred, imho

gundalow commented 6 years ago

Code is complete (thanks @ganeshrn) Only thing left before closing this is to add some details to https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/porting_guide_2.5.rst#networking

gundalow commented 6 years ago

For reference, new packages are live and can be found in https://github.com/ansible/ansible/tree/devel/lib/ansible/module_utils/network

ganeshrn commented 6 years ago

Done as part of PR https://github.com/ansible/ansible/pull/33452