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

Doc: Adding contribution guide for eos_cli_config_gen #4730

Open
wants to merge 16 commits into
base: devel
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
<!--
~ Copyright (c) 2023-2024 Arista Networks, Inc.
~ Use of this source code is governed by the Apache License 2.0
~ that can be found in the LICENSE file.
-->

# Contribution Guide for `eos_cli_config_gen` role
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

This document outlines the steps and checklist for contributing to the `eos_cli_config_gen` role under Arista AVD.

## Steps to add a new feature to eos_cli_config_gen role
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

### Prepare development environment

Follow the [Development Tooling Guide](https://avd.arista.com/5.0/docs/contribution/development-tooling.html).
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

### Schema creation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add an overview of whats needed as a list before this like

  • schema
  • templates (eos and documentation)
  • molecule (tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a list of steps


Add the schema for new feature as per EOS CLI to the appropriate schema fragments file in the `pyavd/_eos_cli_config_gen/schema/schema_fragments` directory or create a new schema file if adding a top-level feature.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

Please refer to the schema documentation for details on the various keys in the schema: [Schema Documentation](https://avd.arista.com/5.0/docs/contribution/input-variable-validation.html#schema-details).

#### Schema Guidelines

1. **Primary Key Placement:** For list-type data-models, place primary keys at the top, for readability.
2. **Key Naming:**
- Follow EOS CLI for key names, when creating new schema keys.
- Use plural for keys that represent multiple elements (e.g., sample_policies).
3. **Descriptions:**
- Only add descriptions to the keys when they provide additional context beyond the key name.
- Refer Arista documentation for description content.
- Ensure all descriptions end with punctuation.
- Highlight the key names in description, like - `<key_name>`.
4. **Type Conversion:** Add `convert_types: [str]` for `type: int` keys.
ClausHolbechArista marked this conversation as resolved.
Show resolved Hide resolved
5. **Defaults:** Avoid using `defaults` in eos_cli_config_gen.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved
6. **Min/Max:** Specify min/max values in the schema if they are defined in the EOS CLI.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add something about valid values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add the the use of required key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it, as suggested by claus on other PRs.

### Creating Jinja2 Templates

Edit the appropriate jinja2 templates in `pyavd/_eos_cli_config_gen/j2templates/eos` and `pyavd/_eos_cli_config_gen/j2templates/documentation` to generate the desired configuration and documentation.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

Add new jinja2 template if adding a top-level feature, also modify the `pyavd/_eos_cli_config_gen/j2templates/eos-intended-config.j2` and `pyavd/_eos_cli_config_gen/j2templates/eos-device-documentation.j2` to add these new templates, trying to respect the order in the EOS CLI.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

#### Jinja2 Templates Guidelines

1. **Code Indentation:** Keep less indented code to improve readability.
2. **Variable Naming:** Use meaningful variable names.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved
3. **Use AVD filters:** Use AVD filters for code optimization - [AVD Filters](https://avd.arista.com/5.0/docs/plugins/Filter_plugins/add_md_toc.html).
4. **Natural Sorting:** Use `arista.avd.natural_sort` for sorting the `for loops` after checking on EOS CLI.
5. **Defined Checks:**
- Avoid `arista.avd.defined` check for parent keys when directly checking for child keys.
- Avoid `arista.avd.defined` check for primary and required keys.
- Avoid`arista.avd.defined` check when using `arista.avd.default()` and `arista.avd.natural_sort` filters.
6. **Password Security:** Avoid displaying passwords in the documentation template and use the `arista.avd.hide_passwords` filter to hide it.
7. **Config Order:** Ensure the order and indentation of configuration matches EOS CLI.
8. **Exclamation Marks:** Place exclamation marks `!` correctly as per the EOS running-config.
9. Along with EOS config template, update the documentation template as well (if required).
10. Implement only commands visible in running-config of the EOS device.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved
11. Validate the template using j2lint tool, run `pre-commit run j2lint --all`.

### Build Schemas and Documentation

Run `pre-commit run schemas --all` to re-generate the eos_cli_config_gen schema with any modifications. This command should be executed every time the schema is changed, even if only a description is updated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Starting AVD 5.1 - it will happen automaticaly when running molecule but will be required if trying with an external repo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically if you run from source (which would happen in molecule / if you run the collection from its github repo) you would not need to do this when running through Ansible. When using pyavd you need to.

I think it is fair to state something like "When developing a feature you should be running from source using Ansible (either via molecule or from a test repo) and so the schemas and templates should be auto-recompiled during the "Verify Requirements" step as per https://avd.arista.com/stable/docs/contribution/development-tooling.html#running-from-source. When running pyavd or if you need to recompile for another reason use pre-commit run schemas --all and / or pre-commit run templates --all."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

It also updates the documentation with new options.

### Add Molecule Tests

Add some molecule tests in the `ansible_collections/arista/avd/molecule/eos_cli_config_gen` scenario exercising the new knob configuration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain how to add to hostX/xxx.yml and adding to subsequent hosts to test extra combination

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


When marking any key as "deprecated," move the related tests to the `eos_cli_config_gen_deprecated_vars` molecule scenario and add any missing tests if necessary.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved

### Run Molecule Tests

Run `molecule converge -s eos_cli_config_gen` from the path `ansible_collections/arista/avd/` to execute the molecule tests locally and generate the new expected configuration and documentation for newly added test-cases.

Check the PyAVD test coverage report by running `tox -e coverage,report` and work on improving the coverage where possible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be hard to follow given the templates are compiled and we are not explaining how to work from there (which is not straighforward)


### Update Documentation

If the proposed feature requires any changes to the documentation, make sure to update it accordingly.
If there are any breaking changes introduced, document them in the porting guide.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ClausHolbechArista / @carlbuchmann - porting guide for major releases / release-notes for minor and maintenance releases?


### Run Pre-commit Checks

Run all pre-commit checks `pre-commit run --all` to ensure that all files added or modified are correctly following the coding standards and formatting rules.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Run all pre-commit checks `pre-commit run --all` to ensure that all files added or modified are correctly following the coding standards and formatting rules.
Run all pre-commit checks `pre-commit run` to ensure that all files added or modified are correctly following the coding standards and formatting rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commit run checks staged files only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. But pre-commit run --all takes forever :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it still reads --all is it expected? (conversation was resolved)

Running these checks also ensures that the changes pass CI checks.

### Self Review The Changes

Before pushing the changes to GitHub, carefully review all the modifications.
Confirm that all new features work as intended and that existing features are unaffected. Once satisfied, proceed to push the changes to the repository.

## Checklist to review an eos_cli_config_gen PR

1. Check that all the CI checks are passing.
2. If the PR addresses an issue raised in the repository, confirm that the issue is fully resolved by the PR.
3. Refer to the Arista documentation for a deeper understanding of the proposed feature.
4. Verify that the schema adheres to EOS CLI and all relevant guidelines mentioned above.
5. Ensure that the min/max values are specified in the schema if they are defined in the EOS CLI.
MaheshGSLAB marked this conversation as resolved.
Show resolved Hide resolved
6. Ensure that Jinja2 templates follow all the guidelines mentioned above.
7. Check that the template generates valid configuration and documentation, maintaining the same configuration order and indentation as EOS CLI.
8. Check out the PR in a local IDE and run all tests to ensure functionality.
Shivani-gslab marked this conversation as resolved.
Show resolved Hide resolved
9. Test the generated configuration through EAPI or CVP in the ATD lab.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not everybody may have an ATD lab - you can state in a relevant lab (if availbale otherwise indicate you could not verify in a lab in your review comment.)

Suggested change
9. Test the generated configuration through EAPI or CVP in the ATD lab.
9. Test the generated configuration through EAPI or CVP in the ATD lab.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

10. If providing code suggestions, test them locally to ensure that your proposals work as intended.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add something to state to add inline suggestions whenever possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

11. Check that the molecule tests are added for the new feature.
12. If any keys are marked as deprecated, ensure that the associated tests are moved to the `eos_cli_config_gen_deprecated_vars` scenario.
13. If the proposed feature requires any changes to the documentation, ensure that it is updated accordingly.
14. Approve the PR if everything looks good.
Loading