Add test for managedclusteradopt controller

[troy0820]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Adds testing to the controllers package

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 #
Part of #3649
Special notes for your reviewer:

TODOs:

• squashed commits
• includes documentation
• adds unit tests
• cherry-pick candidate

Release note:

None

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.27%. Comparing base (b32f0c6) to head (5fd05b1).
Report is 48 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5538      +/-   ##
==========================================
+ Coverage   52.93%   53.27%   +0.33%     
==========================================
  Files         272      272              
  Lines       29485    29485              
==========================================
+ Hits        15607    15707     +100     
+ Misses      13061    12954     -107     
- Partials      817      824       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[troy0820]

cluster-api-provider-azure/controllers/managedclusteradopt_controller.go

Lines 172 to 181 in b32f0c6

	managedCluster = &asocontainerservicev1.ManagedCluster{
	TypeMeta: metav1.TypeMeta{
	APIVersion: asocontainerservicev1.GroupVersion.Identifier(),
	Kind: "ManagedCluster",
	},
	ObjectMeta: metav1.ObjectMeta{
	Name: managedCluster.Name,
	},
	Spec: managedCluster.Spec,
	}

It doesn't look like we are doing anything with this resource after we patch it, then we create the AzureASOManagedControlPlane. Is this necessary?

[troy0820]

/retest

[jackfrancis]

/assign @nojnhuh

[nojnhuh]

cluster-api-provider-azure/controllers/managedclusteradopt_controller.go

Lines 172 to 181 in b32f0c6

	managedCluster = &asocontainerservicev1.ManagedCluster{
	TypeMeta: metav1.TypeMeta{
	APIVersion: asocontainerservicev1.GroupVersion.Identifier(),
	Kind: "ManagedCluster",
	},
	ObjectMeta: metav1.ObjectMeta{
	Name: managedCluster.Name,
	},
	Spec: managedCluster.Spec,
	}

It doesn't look like we are doing anything with this resource after we patch it, then we create the AzureASOManagedControlPlane. Is this necessary?

I'm not sure I'm following your question, but the managedCluster variable is referenced in the asoManagedControlPlane that gets created. And that AzureASOManagedControlPlane resource doesn't get touched afterwards by this controller because that's what it's designed to do since adoption is more of a one-time thing. https://capz.sigs.k8s.io/managed/adopting-clusters#option-1-using-the-new-azureasomanaged-api

[nojnhuh]

Overall I think an e2e test for this is going to be much more valuable than unit testing the controller. Is that something you're up for?

[troy0820]

I'm not sure I'm following your question, but the managedCluster variable is referenced in the asoManagedControlPlane that gets created. And that AzureASOManagedControlPlane resource doesn't get touched afterwards by this controller because that's what it's designed to do since adoption is more of a one-time thing. https://capz.sigs.k8s.io/managed/adopting-clusters#option-1-using-the-new-azureasomanaged-api

@nojnhuh

In the logic,

	managedClusterBefore := managedCluster.DeepCopy()
	managedCluster.Spec.AgentPoolProfiles = nil
	err = r.Patch(ctx, managedCluster, client.MergeFrom(managedClusterBefore))
	if err != nil {
		return ctrl.Result{}, err
	}

	managedCluster = &asocontainerservicev1.ManagedCluster{
		TypeMeta: metav1.TypeMeta{
			APIVersion: asocontainerservicev1.GroupVersion.Identifier(),
			Kind:       "ManagedCluster",
		},
		ObjectMeta: metav1.ObjectMeta{
			Name: managedCluster.Name,
		},
		Spec: managedCluster.Spec,
	}

we patch the managedcluster after we make the spec.AgentPoolProfileds = nil.

Then we shadow the variable managedCluster to then put on the Object: managedCluster part of the ASOManagedControlPlane.Spec

Do we need to create a new object when we have the managedCluster already even the before one before the patch?

Overall I think an e2e test for this is going to be much more valuable than unit testing the controller. Is that something you're up for?

I definitely agree and can see if I can put that in another PR. I have to see how long that will take me. The unit test here can still be valuable as future changes will allow the test to fail if the functionality changes.

[nojnhuh]

Since managedCluster comes from the API, it contains lots of things that don't need to be in the AzureASOManagedControlPlane spec. Those things at best are wasteful and an eyesore (status), and at worst they might totally break reconciliation (metadata.resourceVersion). Creating a new object throws away all those things from the CAPZ spec except what CAPZ needs. This doesn't remove anything from the live ManagedCluster resource, it only pares down what fields CAPZ claims ownership over.

[troy0820]

Was this waiting on the integration test? If so, I can add it but didn't know if you wanted that in the same PR. @nojnhuh

[willie-yao]

/assign

[willie-yao]

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 42480158eba30e3a8f232e085db095a7ceba3018

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: willie-yao

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 [willie-yao]

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

[willie-yao]

Changes look good to me, thank you @troy0820! We might want to open a new issue to track an e2e test for the adoption flow. Would you be able to do that?

[troy0820]

Changes look good to me, thank you @troy0820! We might want to open a new issue to track an e2e test for the adoption flow. Would you be able to do that?

Yeah I can do that.

/retest

[nojnhuh]

Thanks @troy0820 for the test and @willie-yao for getting this in!