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 'preference' attribute to aci_l3out_static_routes_nexthop (DCNE-303) #722

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yamauchih1213
Copy link

Summary

This PR adds a new attribute, preference, to the aci_l3out_static_routes_nexthop module, enabling the configuration of a preference value for next-hop IP addresses in static routes. This change addresses a common use case where routing preferences need to be explicitly set for better control over routing decisions.

Changes

  • Introduced the preference attribute to aci_l3out_static_routes_nexthop
  • Added test cases for validating the new functionality

Notes

  • The preference attribute is optional. If omitted, the API will use the default value of 0.
  • Ensured backward compatibility by not altering existing functionality without a preference value.

@akinross akinross added the jira-sync Sync this issue to Jira label Feb 7, 2025
@github-actions github-actions bot changed the title Add 'preference' attribute to aci_l3out_static_routes_nexthop Add 'preference' attribute to aci_l3out_static_routes_nexthop (DCNE-303) Feb 7, 2025
Comment on lines 51 to 54
preference:
description:
- The administrative preference value for the nexthop
type: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you document the default value apic sets when not provided and the allowed integer range, think it is 1-255 if not mistaken. Could you validate this and adjust the docs suggestion below.

Suggested change
preference:
description:
- The administrative preference value for the nexthop
type: int
preference:
description:
- The administrative preference value for the nexthop.
- The APIC defaults to 0 when unset during creation.
- The value must be between 1 and 255.
type: int

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback!

After further investigation, I found that the UI tooltip confirms that 0 is also a permitted value, along with "defaultValue" and "unspecified". Additionally, in actual testing, specifying 0 worked without issues, while 256 resulted in an error.

Based on this, I will update the documentation to reflect that 0 is a valid option, in addition to 1-255. Please let me know if you have any concerns about this approach.

Copy link
Collaborator

@akinross akinross Feb 7, 2025

Choose a reason for hiding this comment

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

Thanks for checking. Sure 0-255 is fine, if you can adjust that it is good.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Please confirm. Thank you!

- name: Verify default preference
ansible.builtin.assert:
that:
- add_nh_no_pref.is_changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason to use is_changed? could you keep consistent with the rest?

Suggested change
- add_nh_no_pref.is_changed
- add_nh_no_pref is changed

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your confirmation. It's my mistake. I will correct it as you pointed out. excuse me.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. Please confirm. Thank you!

Copy link
Collaborator

@akinross akinross left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@gmicol gmicol left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@samiib samiib left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@shrsr shrsr left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira-sync Sync this issue to Jira
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants