💎 cloud: Refactor Subnets scope to interface #768

cpanato · 2020-07-08T17:43:16Z

What this PR does / why we need it:
Follow up from #716 , this PR applies the same pattern to Subnets.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes partially #757

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

 cloud: Refactor Subnets scope to interface

cpanato · 2020-07-08T17:44:41Z

Opening the PR to get some feedback because I'm not 100% sure if I did that correctly.
Also when all is good will add a few more tests to cover the spec array

cc @CecileRobertMichon @devigned
thanks in advance for taking the time to review this PR.

cloud/scope/machine.go

controllers/azurecluster_reconciler.go

cloud/services/subnets/subnets_test.go

cpanato · 2020-07-10T08:08:51Z

@CecileRobertMichon done the missing parts and add a few more test cases. PTAL
thanks for your review

devigned

Looks good. Just a couple points of feedback.

controllers/azurecluster_reconciler.go

cloud/services/subnets/subnets.go

controllers/azurecluster_reconciler.go

cloud/services/subnets/subnets.go

CecileRobertMichon

lgtm, /assign @devigned for another review on the getExisting subnet changes

devigned

Subnet Reconcile is a little tough to follow. Perhaps, a light bit of refactoring would help.

devigned · 2020-07-14T19:01:44Z

cloud/services/subnets/subnets.go

+func (s *Service) Reconcile(ctx context.Context) error {
+	for i, subnetSpec := range s.Scope.SubnetSpecs() {
+		existingSubnet, err := s.getExisting(ctx, s.Scope.Vnet().ResourceGroup, subnetSpec)
+		if err == nil {


What do you think about the following refactor to clean things up a bit:

for i, subnetSpec := range s.Scope.SubnetSpecs() { existingSubnet, err := s.getExisting(ctx, s.Scope.Vnet().ResourceGroup, subnetSpec) switch { case err != nil && !azure.ResourceNotFound(err): // not found case err != nil: // stuff for existing subnet case !s.Scope.Vnet().IsManaged(s.Scope.ClusterName()): // why does is managed need `s.Scope.ClusterName()`? // if the vnet is unmanaged, we expect all subnets to be created as well default: // stuff to create new subnet }

Just wanted to call out: "why does is managed need s.Scope.ClusterName()?" Seems odd to have to pass in s.Scope.ClusterName() when calling a func off of hierarchical descendant of s.Scope.

I think it's because IsManaged is on the infra VnetSpec, not the scope itself.
this could also read:

vnet := s.Scope.Vnet() // return type *infrav1.VnetSpec if !vnet.IsManagedBy(s.Scope.ClusterName()) { ... }

we could change this into s.Scope.IsVnetManaged() though

Agree it seems a bit odd like this

made all the requested changes, please take a look again :) thanks for all inputs!

cpanato · 2020-07-17T10:23:40Z

@CecileRobertMichon was missing the / :)

CecileRobertMichon

/lgtm
/approve

CecileRobertMichon

/lgtm

cpanato · 2020-07-17T18:17:34Z

Looks like my rebase did went well. Will fix that tomorrow

cpanato · 2020-07-18T15:23:19Z

/test pull-cluster-api-provider-azure-e2e

CecileRobertMichon

One more comment based on #806 to fix parallelism in the tests

cloud/services/subnets/subnets_test.go

k8s-ci-robot · 2020-07-21T19:09:34Z

@cpanato: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`299e266`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

CecileRobertMichon

/lgtm
/approve

k8s-ci-robot · 2020-07-21T20:42:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 8, 2020

k8s-ci-robot requested review from awesomenix and justaugustus July 8, 2020 17:43

CecileRobertMichon reviewed Jul 8, 2020

View reviewed changes

cloud/scope/machine.go Outdated Show resolved Hide resolved

cpanato force-pushed the GH-757-subnet branch from 1ad59ab to 91f6469 Compare July 8, 2020 17:45

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

cpanato force-pushed the GH-757-subnet branch from 91f6469 to c0f106b Compare July 8, 2020 18:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2020

cpanato changed the title ~~💎 WIP cloud: Refactor Subnets scope to interface~~ WIP 💎 cloud: Refactor Subnets scope to interface Jul 8, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2020

CecileRobertMichon reviewed Jul 9, 2020

View reviewed changes

controllers/azurecluster_reconciler.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Jul 9, 2020

View reviewed changes

controllers/azurecluster_reconciler.go Outdated Show resolved Hide resolved

CecileRobertMichon reviewed Jul 9, 2020

View reviewed changes

cloud/services/subnets/subnets_test.go Outdated Show resolved Hide resolved

cpanato force-pushed the GH-757-subnet branch from c0f106b to cff8cbe Compare July 10, 2020 08:07

cpanato changed the title ~~WIP 💎 cloud: Refactor Subnets scope to interface~~ 💎 cloud: Refactor Subnets scope to interface Jul 10, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2020

devigned suggested changes Jul 10, 2020

View reviewed changes

controllers/azurecluster_reconciler.go Outdated Show resolved Hide resolved

cloud/services/subnets/subnets.go Outdated Show resolved Hide resolved

controllers/azurecluster_reconciler.go Outdated Show resolved Hide resolved

cpanato force-pushed the GH-757-subnet branch from cff8cbe to a4ed904 Compare July 10, 2020 15:01

CecileRobertMichon reviewed Jul 10, 2020

View reviewed changes

cloud/services/subnets/subnets.go Outdated Show resolved Hide resolved

cpanato force-pushed the GH-757-subnet branch 2 times, most recently from 14a22de to 1d79586 Compare July 14, 2020 15:16

CecileRobertMichon approved these changes Jul 14, 2020

View reviewed changes

devigned suggested changes Jul 14, 2020

View reviewed changes

cpanato force-pushed the GH-757-subnet branch from 1d79586 to 2cc185d Compare July 15, 2020 08:31

CecileRobertMichon reviewed Jul 17, 2020

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Jul 17, 2020

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 17, 2020

cpanato force-pushed the GH-757-subnet branch from 4bb5d00 to fce3f64 Compare July 17, 2020 17:51

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 17, 2020

CecileRobertMichon approved these changes Jul 17, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 17, 2020

cpanato force-pushed the GH-757-subnet branch from fce3f64 to 92bcc68 Compare July 18, 2020 13:56

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2020

cpanato requested a review from CecileRobertMichon July 18, 2020 14:20

CecileRobertMichon suggested changes Jul 20, 2020

View reviewed changes

cloud/services/subnets/subnets_test.go Show resolved Hide resolved

cloud/services/subnets/subnets_test.go Show resolved Hide resolved

cpanato force-pushed the GH-757-subnet branch from 92bcc68 to 5745d73 Compare July 21, 2020 06:45

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2020

cpanato force-pushed the GH-757-subnet branch from 5745d73 to aee3831 Compare July 21, 2020 18:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2020

cloud: Refactor Subnets scope to interface

299e266

cpanato force-pushed the GH-757-subnet branch from aee3831 to 299e266 Compare July 21, 2020 19:06

CecileRobertMichon reviewed Jul 21, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2020

k8s-ci-robot merged commit eddf931 into kubernetes-sigs:master Jul 21, 2020

k8s-ci-robot added this to the v0.4.7 milestone Jul 21, 2020

cpanato deleted the GH-757-subnet branch July 22, 2020 05:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

💎 cloud: Refactor Subnets scope to interface #768

💎 cloud: Refactor Subnets scope to interface #768

cpanato commented Jul 8, 2020 •

edited

Loading

cpanato commented Jul 8, 2020

cpanato commented Jul 10, 2020

devigned left a comment

CecileRobertMichon left a comment

devigned left a comment

devigned Jul 14, 2020 •

edited

Loading

devigned Jul 14, 2020

CecileRobertMichon Jul 14, 2020

CecileRobertMichon Jul 14, 2020

cpanato Jul 15, 2020

cpanato commented Jul 17, 2020

CecileRobertMichon left a comment

CecileRobertMichon left a comment

cpanato commented Jul 17, 2020

cpanato commented Jul 18, 2020

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 21, 2020 •

edited

Loading

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 21, 2020

💎 cloud: Refactor Subnets scope to interface #768

💎 cloud: Refactor Subnets scope to interface #768

Conversation

cpanato commented Jul 8, 2020 • edited Loading

cpanato commented Jul 8, 2020

cpanato commented Jul 10, 2020

devigned left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

devigned left a comment

Choose a reason for hiding this comment

devigned Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

devigned Jul 14, 2020

Choose a reason for hiding this comment

CecileRobertMichon Jul 14, 2020

Choose a reason for hiding this comment

CecileRobertMichon Jul 14, 2020

Choose a reason for hiding this comment

cpanato Jul 15, 2020

Choose a reason for hiding this comment

cpanato commented Jul 17, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

cpanato commented Jul 17, 2020

cpanato commented Jul 18, 2020

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 21, 2020 • edited Loading

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 21, 2020

cpanato commented Jul 8, 2020 •

edited

Loading

devigned Jul 14, 2020 •

edited

Loading

k8s-ci-robot commented Jul 21, 2020 •

edited

Loading