update SDK to track2 in ensureServiceEndpoints #3618
Conversation
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
405a124
to
b6d3ae4
Compare
|
Please rebase pull request. |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
b6d3ae4
to
cde1d7c
Compare
| for _, subnetId := range subnetIds { | ||
| r, err := arm.ParseResourceID(subnetId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| subnet, err := m.armSubnets.Get(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, nil) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| shouldUpdate := addEndpointsToSubnet(api.SubnetsEndpoints, &subnet.Subnet) | ||
| if !shouldUpdate { | ||
| continue | ||
| } | ||
| err = m.armSubnets.CreateOrUpdateAndWait(ctx, r.ResourceGroupName, r.Parent.Name, r.Name, subnet.Subnet, nil) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Just a comment off the top of my head.
There is nothing wrong with this, but a slightly different approach that could also work would be to divide that in the following way:
- first get all the subnets that should be updated (in a function returning a slice)
- pass those subnets that should be updated (return value from the previous function) to another function that would simply do CreateOrUpdateAndWait for each value in the slice.
I don't think is much better than what the PR does, but it sticks to the principle of not iterating + doing stuff with what you are iterating, but instead, first do some processing and once that is processed update the values in Azure. Not asking to change that, just a comment about a pattern that sometimes can be useful, even to simplify unit tests in some cases (as you don't need to mock m.armSubnets because you just want to test the function that does the business logic which is filtering the subnets).
🙂
|
Please rebase pull request. |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
cde1d7c
to
88a1b27
Compare
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
88a1b27
to
e5ba65f
Compare
|
Please rebase pull request. |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
e5ba65f
to
590ccaf
Compare
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
2 times, most recently
from
639eaa5
to
3da6a1e
Compare
|
Please rebase pull request. |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
3da6a1e
to
8a66831
Compare
|
Please rebase pull request. |
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
8a66831
to
682af6a
Compare
bitoku
force-pushed
the
ensureServiceEndpoints-track2
branch
from
682af6a
to
70b493c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Please rebase pull request. |
Which issue this PR addresses:
Fixes a part of https://issues.redhat.com/browse/ARO-4665 and https://issues.redhat.com/browse/ARO-7316
This is blocked by #3616
What this PR does / why we need it:
This PR makes
ensureServiceEndpointsaction use track2 SDK.It also contains the change of subnet manager, and I'm going to remove the subnet manager completely in the future.
I want to remove the subnet manager because some methods (
CreateOrUpdateFromIds,CreateOrUpdateSubnets, andGetHighestFreeIP) are used only once, and I think it adds unnecessary complexity.The merit to have it which I can think of is that it can be mocked and can keep the unit tests simple, but after removing it, the test is not that complex and I think the suggested change would make the behaviour clearer.
Any comments about removal of the subnet manager are welcome.
Test plan for issue:
unit tests for ensureServiceEndpoints.
e2e
Is there any documentation that needs to be updated for this PR?
tech debt cleanup N/A
How do you know this will function as expected in production?
cluster installation test