Skip to content

Conversation

@manoj-nutanix
Copy link
Contributor

What problem does this PR solve?:
Follow up PR of https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pull/1001/files#r1884249547

Which issue(s) this PR fixes:
Fixes https://jira.nutanix.com/browse/NCN-104787

How Has This Been Tested?:

Special notes for your reviewer:

@dlipovetsky
Copy link
Contributor

dlipovetsky commented Jan 10, 2025

I disagree with the suggestion in https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pull/1001/files#r1884249547. I believe we should not make this change. The function becomes more difficult to use, because it is no longer clear what a valid input is.

The comment mentions 3 use cases (IP in an IP range, IP in a CIDR, IP in IP range where the start and end IP are the same). I see no need to avoid multiple functions for each use case. Our API fields have (and should have) unambiguous types. We do not have any field that can be an IP, an IP range, or a CIDR. Therefore, validation functions can choose the correct function to call.

We can write a straightforward function to check an IP in a CIDR. (The comment also mentions a range of one IP; in that case, it's possible to make a /32 CIDR from the IP).

@manoj-nutanix
Copy link
Contributor Author

I disagree with the suggestion in https://github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pull/1001/files#r1884249547. I believe we should not make this change. The function becomes more difficult to use, because it is no longer clear what a valid input is.

The comment mentions 3 use cases (IP in an IP range, IP in a CIDR, IP in IP range where the start and end IP are the same). I see no need to avoid multiple functions for each use case. Our API fields have (and should have) unambiguous types. We do not have any field that can be an IP, an IP range, or a CIDR. Therefore, validation functions can choose the correct function to call.

We can write a straightforward function to check an IP in a CIDR. (The comment also mentions a range of one IP; in that case, it's possible to make a /32 CIDR from the IP).

ik it's not idiomatic but I've seen suggestion to create such unnecessary wrapper funcs in past so created a separate PR otherwise it would have delayed original PR if we kept discussing the suggestion on original PR. It's okay skip this change if we have agreement.

@manoj-nutanix
Copy link
Contributor Author

closing in favour of #1007 (comment) .. we can revisit if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants