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

Inconsistent parameter naming for ip dataplane learning (DCNE-115) #1152

Open
bardahlm opened this issue Feb 5, 2024 · 5 comments
Open

Inconsistent parameter naming for ip dataplane learning (DCNE-115) #1152

bardahlm opened this issue Feb 5, 2024 · 5 comments
Labels
enhancement jira-sync Sync this issue to Jira

Comments

@bardahlm
Copy link

bardahlm commented Feb 5, 2024

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

n/a

APIC version and APIC Platform

n/a

Affected Resource(s)

  • aci_bridge_domain
  • aci_subnet
  • aci_vrf

Terraform Configuration Files

These are examples for configuring ip dataplane learning for vrf, bd or subnet. vrf and subnet use ip_data_plane_learning = "disabled"/"enabled while bd use ip_learning = "yes"/"no"

resource "aci_bridge_domain" "foobridge_domain" {
[...]
        ip_learning                 = "yes"
[...]
}

resource "aci_subnet" "foo_epg_subnet_disable_dp_learning" {
[...]
  ip_data_plane_learning = "disabled"
}
resource "aci_vrf" "foovrf" {
[...]
  ip_data_plane_learning = "enabled"
[...]
}

Can these get equal parameters and descriptions if they do the exact same but on different places in the network?

Important Factoids

References

Document describing Dataplane IP Address Learning: https://www.cisco.com/c/en/us/td/docs/dcn/aci/apic/6x/l3-configuration/cisco-apic-layer-3-networking-configuration-guide-60x/dataplane-ip-learning-per-vrf-layer3-config-60x.pdf

@akinross
Copy link
Collaborator

akinross commented Feb 5, 2024

Hi @bardahlm,

Thank you for raising the inconsistency, the reason for this is that we have been following the model itself. The UI also follows the yes/no (db) and disabled/enabled (vrf), so we will need to discuss if we should adjust this. I will add it to the to do list.

@bond
Copy link

bond commented Feb 26, 2024

Yes/no, enabled/disabled, true/false should use the boolean datatype in Terraform. This is what this data-type is meant for, and makes it possible to assign the result of an expression or a true-false variable to the value.

@akinross
Copy link
Collaborator

Hi @bond,

Thank you for your constructive comment with a improvement suggestion for the provider.

From my understanding the choice has been made in the past to keep the input as much as possible consistent with the model and UI input which are not boolean values but string based values. I understand your comment, and do not disagree that the boolean data-type is meant for true/false behaviour. However this choice was not made in the past, and the question we need to ask ourselves now I think is different.

Is it worth to create a potential breaking change when this can be solved with conditional expressions in HCL?

See documentation for below example:

condition ? true_val : false_val

Furthermore I would argue that the behaviour you are describing is a little bit different from the issue that is raised here. We could create a new issue to track this item and see if there is more ask for this change in behaviour.

@bond
Copy link

bond commented Feb 26, 2024

Hi, thanks for your reply.

I'm now using conditional expressions like:

resource "aci_logical_node_to_fabric_node" "this" {
...
rtr_id_loop_back        = var.create_loopbacks ? "yes" : "no"
...
}

but would much prefer to be able to use the power of the HCL-language, in example:
rtr_id_loop_back = var.create_loopbacks

or with the result of an expression:
rtr_id_loop_back = var.dont_create_loopbacks == false

I see your argument and it would be a breaking change, and breaking changes hurt. But how long should one avoid improvements if, and will the change hurt less in the future? Ofcourse it would need to be discussed pros/cons like you say.

@akinross
Copy link
Collaborator

Hi @bond,

I have created a separate issue to track this specific behaviour. This way we can see if others also prefer this behaviour.

@samiib samiib added the jira-sync Sync this issue to Jira label Aug 14, 2024
@github-actions github-actions bot changed the title Inconsistent parameter naming for ip dataplane learning Inconsistent parameter naming for ip dataplane learning (DCNE-115) Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement jira-sync Sync this issue to Jira
Projects
None yet
Development

No branches or pull requests

4 participants