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(eos_cli_config_gen, eos_designs): Consistent descriptions for BGP AS schema fields re asdot notation #3618

Conversation

ClausHolbechArista
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista commented Feb 12, 2024

Change Summary

  • Improve descriptions for BGP AS schema fields re asdot notation

We should not implement convert_types from float since it would introduce a risk when the user inputs 65000.100, we would configure 65000.1. We already have that a few places, so to avoid breaking changes, this is kept in this PR.

Component(s) name

arista.avd.eos_designs
arista.avd.eos_cli_config_gen

@ClausHolbechArista ClausHolbechArista requested review from a team as code owners February 12, 2024 18:21
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: Documentation role Updated role: eos_designs issue related to eos_designs role labels Feb 12, 2024
Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

The added description is missing under network service: <network_services_keys.name>.[].vrfs.[].bgp_peers.[].remote_as.

@ClausHolbechArista ClausHolbechArista changed the title Doc(eos_cli_config_gen): Improve descriptions for BGP AS schema fields re asdot notation Fix(eos_cli_config_gen, eos_designs): Consistent conversion and descriptions for BGP AS schema fields re asdot notation Feb 14, 2024
@ClausHolbechArista
Copy link
Contributor Author

The added description is missing under network service: <network_services_keys.name>.[].vrfs.[].bgp_peers.[].remote_as.

The scope was only eos_cli_config_gen, but I have fixed it for eos_designs as well now, and I actually found a few places where we converted from float. So since I change that I have changed this to a fix instead of doc.

@ClausHolbechArista ClausHolbechArista marked this pull request as draft February 14, 2024 09:05
@ClausHolbechArista ClausHolbechArista changed the title Fix(eos_cli_config_gen, eos_designs): Consistent conversion and descriptions for BGP AS schema fields re asdot notation Doc(eos_cli_config_gen, eos_designs): Consistent descriptions for BGP AS schema fields re asdot notation Feb 15, 2024
@ClausHolbechArista ClausHolbechArista marked this pull request as ready for review February 15, 2024 11:47
@carlbuchmann carlbuchmann added this to the v4.6.0 milestone Feb 15, 2024
Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

The *must* is not rendered properly. I think it was fine when I first reviewed.

image

@carlbuchmann
Copy link
Member

The *must* is not rendered properly. I think it was fine when I first reviewed.

image

That is to be expected in the YAML output. It is italic in the MD table as expected.But perhaps we want bold?

Copy link
Member

@carlbuchmann carlbuchmann left a comment

Choose a reason for hiding this comment

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

LGTM, small comment on format

Copy link
Contributor

@chetryan chetryan left a comment

Choose a reason for hiding this comment

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

LGTM - much clearer descriptions than before.

@ClausHolbechArista ClausHolbechArista merged commit 03555df into aristanetworks:devel Feb 16, 2024
41 checks passed
sugetha24 pushed a commit to sugetha24/ansible-avd that referenced this pull request Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants