[ARO-15009] fix: add logic to skip subnet update under certain conditions

[scottd018]

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-15009

What this PR does / why we need it:

This commit addresses the need for the subnet/write permissions. Currently, in all scenarios, the subnet/write permission is used via the subnet.CreateOrUpdate method. This change adds logic to determine if the values are already set to Disabled and skips running the subnet.CreateOrUpdate method if it is already set appropriately. This will save the need to make an extra API call and also allow users to configure things like Azure policy where subnet/write permissions are not needed.

Closed #4084 in favor of this PR.

Test plan for issue:

Currently awaiting access for the Dev ARO-RP shared environment. Once I get access I can run a full E2E test and comment with the results.

Is there any documentation that needs to be updated for this PR?

No documentation needs to be updated, as the end user experience looks identical.

How do you know this will function as expected in production?

No change to the existing logic is introduced. We are only introducing a skip of the API call to create or update a subnet if it does not currently match a desired stated of "Disabled".

[kimorris27]

The change LGTM, but one thing I missed from my original review - you could add a unit test case validating that CreateOrUpdate is not called when those two attributes both already have the correct values.

[scottd018]

The change LGTM, but one thing I missed from my original review - you could add a unit test case validating that CreateOrUpdate is not called when those two attributes both already have the correct values.

Absolutely can do that. I'll get a new commit added and pushed. I thought about doing it originally, but hadn't seen any test cases for the specific method (maybe I missed them...let me know if there's some specific place you want them otherwise I can add it to the _test.go file like I'm used to doing).

[kimorris27]

The change LGTM, but one thing I missed from my original review - you could add a unit test case validating that CreateOrUpdate is not called when those two attributes both already have the correct values.

Absolutely can do that. I'll get a new commit added and pushed. I thought about doing it originally, but hadn't seen any test cases for the specific method (maybe I missed them...let me know if there's some specific place you want them otherwise I can add it to the _test.go file like I'm used to doing).

The unit tests for the function you tweaked are here:

ARO-RP/pkg/cluster/deploybaseresources_test.go

Line 470 in fb723cf

func TestSetMasterSubnetPolicies(t *testing.T) {

[scottd018]

The change LGTM, but one thing I missed from my original review - you could add a unit test case validating that CreateOrUpdate is not called when those two attributes both already have the correct values.

Absolutely can do that. I'll get a new commit added and pushed. I thought about doing it originally, but hadn't seen any test cases for the specific method (maybe I missed them...let me know if there's some specific place you want them otherwise I can add it to the _test.go file like I'm used to doing).

The unit tests for the function you tweaked are here:

ARO-RP/pkg/cluster/deploybaseresources_test.go

Line 470 in fb723cf

func TestSetMasterSubnetPolicies(t *testing.T) {

@kimorris27 Take a look a 26139fa and ca2b1ca. I added a couple of tests that seem to align with the others that currently exist. Let me know if you need any other changes. Thanks!

[hawkowl]

/azp run ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).