Envtest mocks #1236

mdbooth · 2022-05-06T18:28:33Z

What this PR does / why we need it:

The purpose of this PR is to be able to run envtests using mock clients instead of real ones. This PR achieves that by enhancing Scope to return service clients. We add a new scope Factory interface to generate Scopes. We provide 2 implementations of Factory:

ProviderScopeFactory generates 'real' Scopes which expect to be able to make API calls to a running OpenStack cloud.
MockScopeFactory uses mock clients.

Because Scopes now return clients, the scope package must import those clients. However, clients are currently in their service packages. Services use scopes and must therefore import the scope package. This is a circular dependency. To avoid this we move clients into a separate package.

With the exception of the new factory interface, all other changes are mechanical. This PR does not intentionally introduce any functional changes.

Because of the large amount of code churn I have split the change into multiple commits. These commits each make a single refactor. I suggest that this PR will be simpler to review by commit, as the mechanical code churn in each individual commit is very similar.

/hold

netlify · 2022-05-06T18:28:36Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`bd1aabe`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/63d8e558a946bf0008ad0e5e
😎 Deploy Preview	https://deploy-preview-1236--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

k8s-ci-robot · 2022-05-06T18:28:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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~~ [mdbooth]

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 · 2022-05-06T18:28:59Z

@mdbooth: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-cluster-api-provider-openstack-build
/test pull-cluster-api-provider-openstack-e2e-test
/test pull-cluster-api-provider-openstack-test

The following commands are available to trigger optional jobs:

/test pull-cluster-api-provider-openstack-conformance-test

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-provider-openstack-build
pull-cluster-api-provider-openstack-e2e-test
pull-cluster-api-provider-openstack-test

In response to this:

/test

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.

mdbooth · 2022-05-06T18:29:20Z

/test all

mdbooth · 2022-05-06T19:33:02Z

/retest

mdbooth · 2022-05-06T19:52:30Z

/retest

apricote · 2022-05-10T12:42:43Z

Looks good to me! (adding lgtm after a rebase)

We discussed removing the clientOpts and passing the region through the Scope struct in #1178. At least now it is an implementation detail of the ProviderScopeFactory.

seanschneeweiss · 2022-11-25T08:25:33Z

controllers/openstackcluster_controller.go

@@ -111,18 +111,11 @@ func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Req
 		}
 	}()

-	osProviderClient, clientOpts, projectID, err := provider.NewClientFromCluster(ctx, r.Client, openStackCluster)
+	scope, err := r.ScopeFactory.NewClientScopeFromCluster(ctx, r.Client, openStackCluster, log)


Should we pass the logger as part of the context?

Made this comment a long time ago but didn't submit the review.

I just implemented this to play with it, but I don't think so.

My thinking is that in the controllers we retrieve a logger from controller runtime from the context, then we add context to it with a succession of log = log.WithValues(...). The contract of this call is:

// WithValues returns a new Logger instance with additional key/value pairs.

I don't think we can assume that retrieving the logger again from the context would retrieve the logger customised for the current reconcile.

I'm prepared to be wrong on this, though. Either way, could we come back to it in a separate PR? It should be a fairly easy mechanical change if we decide to do this later.

lentzi90 · 2023-01-31T08:57:01Z

Because of the large amount of code churn I have split the change into multiple commits. These commits each make a single refactor. I suggest that this PR will be simpler to review by commit, as the mechanical code churn in each individual commit is very similar.

Thank you! It was quite nice to review per commit! 🙂
This looks like a good improvement to me.
/lgtm

Move 'provider' from the 'cloud/services.provider' package to 'scope'. This is in preparation for adding ProviderScope.

Scope is going to return clients and therefore has to import it. clients therefore can't import scope as this would be a circular dependency.

scope.Factory is a factory for generating Scopes. We provide two implementations: - ScopeFactory generates a Scope from the provider details contained in either a Machine or a Cluster object. It expects to be able to make API calls to a running OpenStack cloud. - MockScopeFactory generates a Scope which uses mock clients from pkg/clients/mock.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Move all provider-specific code into provider.go.

mdbooth · 2023-01-31T09:55:03Z

Fixed merge conflict with #1451

mdbooth · 2023-01-31T09:55:46Z

/hold cancel

lentzi90

/lgtm
Let's get this merged!

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 6, 2022

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 6, 2022

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 6, 2022

k8s-ci-robot requested review from seanschneeweiss and tobiasgiese May 6, 2022 18:28

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2022

mdbooth force-pushed the envtest_mocks branch 4 times, most recently from 967e5f9 to ae843d6 Compare May 6, 2022 19:29

mdbooth force-pushed the envtest_mocks branch from ae843d6 to 391f09f Compare May 6, 2022 19:52

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

stephenfin force-pushed the envtest_mocks branch from 391f09f to 38dea40 Compare July 1, 2022 11:17

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2022

stephenfin force-pushed the envtest_mocks branch from 38dea40 to 5f68052 Compare July 1, 2022 11:31

mdbooth force-pushed the envtest_mocks branch from 5f68052 to 391f09f Compare July 1, 2022 13:28

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

mdbooth changed the title ~~WIP: Envtest mocks~~ Envtest mocks Jul 1, 2022

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 1, 2022

mdbooth force-pushed the envtest_mocks branch from 391f09f to b490b44 Compare July 1, 2022 13:35

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

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

mdbooth force-pushed the envtest_mocks branch from f78fe2e to 807df96 Compare November 14, 2022 10:42

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

mdbooth mentioned this pull request Nov 21, 2022

✨ Add OpenStack simulator for running envtests #1391

Closed

2 tasks

seanschneeweiss reviewed Nov 29, 2022

View reviewed changes

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

mdbooth force-pushed the envtest_mocks branch from 807df96 to e01fbe2 Compare January 30, 2023 15:37

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

k8s-ci-robot assigned lentzi90 Jan 31, 2023

k8s-ci-robot added 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 Jan 31, 2023

mdbooth and others added 8 commits January 31, 2023 09:50

Move provider into scope

e704e66

Move 'provider' from the 'cloud/services.provider' package to 'scope'. This is in preparation for adding ProviderScope.

Remove import of scope by clients

3cdf332

Scope is going to return clients and therefore has to import it. clients therefore can't import scope as this would be a circular dependency.

Add deleteBastion envTest

34d4adc

Correct typo

36efe72

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Use controller runtime logger in HTTPClient

d6166cd

Rename default scope factory to providerScopeFactory

b05bcd1

Move all provider-specific code into provider.go.

Remove TestServices and replace with MockScopeFactory

bd1aabe

mdbooth force-pushed the envtest_mocks branch from 9a865b2 to bd1aabe Compare January 31, 2023 09:54

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 Jan 31, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2023

lentzi90 reviewed Jan 31, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 31, 2023

k8s-ci-robot merged commit e907456 into kubernetes-sigs:main Jan 31, 2023

stephenfin deleted the envtest_mocks branch January 31, 2023 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Envtest mocks #1236

Envtest mocks #1236

mdbooth commented May 6, 2022 •

edited

Loading

netlify bot commented May 6, 2022 •

edited

Loading

k8s-ci-robot commented May 6, 2022

k8s-ci-robot commented May 6, 2022

mdbooth commented May 6, 2022

mdbooth commented May 6, 2022

mdbooth commented May 6, 2022

apricote commented May 10, 2022

seanschneeweiss Nov 25, 2022

seanschneeweiss Nov 29, 2022

mdbooth Jan 30, 2023

lentzi90 commented Jan 31, 2023

mdbooth commented Jan 31, 2023

mdbooth commented Jan 31, 2023

lentzi90 left a comment

Envtest mocks #1236

Envtest mocks #1236

Conversation

mdbooth commented May 6, 2022 • edited Loading

netlify bot commented May 6, 2022 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

k8s-ci-robot commented May 6, 2022

k8s-ci-robot commented May 6, 2022

mdbooth commented May 6, 2022

mdbooth commented May 6, 2022

mdbooth commented May 6, 2022

apricote commented May 10, 2022

seanschneeweiss Nov 25, 2022

Choose a reason for hiding this comment

seanschneeweiss Nov 29, 2022

Choose a reason for hiding this comment

mdbooth Jan 30, 2023

Choose a reason for hiding this comment

lentzi90 commented Jan 31, 2023

mdbooth commented Jan 31, 2023

mdbooth commented Jan 31, 2023

lentzi90 left a comment

Choose a reason for hiding this comment

mdbooth commented May 6, 2022 •

edited

Loading

netlify bot commented May 6, 2022 •

edited

Loading