[minor_changes]Adding guidelines for Ansible ACI module developement (DCNE-80) #761
Conversation
anvitha-jain
requested review from
akinross,
gmicol,
lhercot,
sajagana,
samiib and
shrsr
as code owners
anvitha-jain
force-pushed
the
module_guide
branch
from
44d838f to
7785013
Compare
github-actions
bot
changed the title
anvitha-jain
force-pushed
the
module_guide
branch
from
7785013 to
1c3eb49
Compare
|
|
||
| .. code-block:: python | ||
|
|
||
| #aci.get_diff(aci_class='l3extRsNodeL3OutAtt') |
There was a problem hiding this comment.
Do we need commented line here?
There was a problem hiding this comment.
yes, currently it's the only way to show diff
| type: str | ||
| choices: [ absent, present, query ] | ||
| default: present | ||
| extends_documentation_fragment: |
There was a problem hiding this comment.
Can we include some sample notes and seealso sections?
If yes, please take care of the other places.
| from ansible_collections.cisco.aci.plugins.module_utils.aci import ACIModule, aci_argument_spec | ||
| from ansible.module_utils.basic import AnsibleModule | ||
|
|
||
| 8. In the main function, the argument_spec variable defines all the arguments necessary for this module and is based on aci_argument_spec. We add all the arguments we defined previously in the documentation section to this variable. In our case, we would add description, prefix, track_policy, preference, and bfd to the section below and remove router_id and router_id_as_loopback. |
There was a problem hiding this comment.
Perhaps we could also cover the types of relationships and go deeper into the other options for type like list and how that would be used and implimented.
| * the object_id (usually the name) | ||
| * the configurable properties of the object | ||
| * the object_id of each parent up to the root (usually the name) | ||
| * The child classes that have a 1-to-1 relationship with the main object don't need their own dedicated module and can be incorporated into the parent module. If the relationship is 1-to-many/many-to-many, this child class will need a dedicated module. |
There was a problem hiding this comment.
For one of the modules I worked on, I think it was a one-to-many case but I did not need to create a new module and still maintained desired functionality. Perhaps there could be some elaboration on the relationships.
| --------------------------------------- | ||
| The following imports are standard across ACI modules: | ||
|
|
||
| .. code-block:: python |
There was a problem hiding this comment.
Perhaps you could also elaborate on the environment that needs to be set before we are able to run a module or tests, including versions for libraries and venv activation and setup(this part might not be necessary just personal recommendation). Perhaps a small paragraph on what the environment needs to look like or how to set up the environment with docker incase someone wants to containerize.
There was a problem hiding this comment.
Requirements to run the module is available in GuitHub in the Readme file section, this file only talks about building the module.
| Let's build a test file for our l3out static routes using the existing test for l3out logical node: | ||
| aci_l3out_logical_node -> aci_l3out_static_routes | ||
|
|
||
| 1. In the **tests** directory of our collection, we have the **integration** directory. The **integration** directory consists of **targets**, which has directories for all the test files of modules that currently exist in our collection. We go to the **targets** directory and copy the aci_l3out_logical_node directory, then paste it in the same directory as aci_l3out_static_routes, which should be the same as the name of our module. Upon opening the directory, we find the main.yml file. We open this file and make the following changes. |
There was a problem hiding this comment.
Do we not need to set up an environment or be in a venv to run the tests aswell?
There was a problem hiding this comment.
This document only refers to the part of building the modules and test cases, how to run/execute them is available in the Readme section of Ansible-ACI github page
| 'status': ['preview'], | ||
| 'supported_by': 'community'} | ||
|
|
||
| DOCUMENTATION = r''' |
There was a problem hiding this comment.
If possible could we talk about black formatting, especially when talking with respect to sanity.
|
|
||
| Steps required to perform tests: | ||
|
|
||
| 1. Ansible uses an inventory file to keep track of which hosts are part of your APIC, and how to reach them for running commands and playbooks using credentials for the APIC. To update the inventory, go to **ansible-aci -> tests -> integration -> inventory.networking** and update the file with the credentials of your APIC. |
There was a problem hiding this comment.
Perhaps you could include some common pitfalls that could be addressed or avoided, also I think formatting standards should also be outlined more explicitly including whitespace and empty lines at the end of modules.
| ), | ||
|
|
||
|
|
||
| 14. The payload function is followed by get_diff(), which is used to get the difference between the proposed and existing configurations of 'ipRouteP'. Here, the aci_class is changed to the class name your module is going to manage. |
There was a problem hiding this comment.
Could you add another example of the get_diff functionality especially when it comes to redundancy of configs.
|
|
||
| Steps required to perform tests: | ||
|
|
||
| 1. Ansible uses an inventory file to keep track of which hosts are part of your APIC, and how to reach them for running commands and playbooks using credentials for the APIC. To update the inventory, go to **ansible-aci -> tests -> integration -> inventory.networking** and update the file with the credentials of your APIC. |
There was a problem hiding this comment.
Also I would elaborate a little on how to create a inventory file for the test cases and with the APIC information, and login information.
solves #278