[OCPCLOUD-1706] Migrate test utils from CPMSO repository #265

RadekManak · 2023-01-10T11:44:37Z

This PR moves test utilities and resource builders from CPMSO repository into this repository. This will allow us to reuse them in other test suites.

Tooling for unit test is created as this repo currently only has e2e tests. This will need to be enabled in openshift/release after this PR merges.
The MachineInfoBuilder is kept in the CPMSO repo. MachineInfo's are used only in CPMSO. Moving the builder over would create cyclic dependency.

damdo · 2023-01-10T15:39:56Z

FYI: I got some more resourcebuilders in this PR, we might want to include them there too, openshift/cluster-control-plane-machine-set-operator#156

JoelSpeed

What is the effect on CPMS of importing these changes? Do we need to make a separate go module to confine the imports to just the testutils module or does it work well as is?

Do we have appropriate linting rules on the testutils?

Do we want to keep the resource builders as a flat structure, or group them by API group/versions as you'd expect for other helpers?

JoelSpeed · 2023-01-10T16:50:04Z

pkg/testutils/resourcebuilder/consts.go

+ // Machine API namespace.
+ OpenshiftMachineAPINamespaceName = "openshift-machine-api"
+
+ testClusterIDValue = "cpms-cluster-test-id"


We probably don't want this to be cpms anymore, where else is it used?

As machine label, machine selector. I have dropped the cpms prefix.

RadekManak · 2023-01-11T15:27:28Z

What is the effect on CPMS of importing these changes? Do we need to make a separate go module to confine the imports to just the testutils module or does it work well as is?

I was able to import from CPMSO without issues.

Do we have appropriate linting rules on the testutils?

The linter is running on the files. (with slightly different settings)

Do we want to keep the resource builders as a flat structure, or group them by API group/versions as you'd expect for other helpers?

Probably good idea to structure it now that it will be imported from other repositories. I have restructured it. Let me know if it looks good to you.

damdo · 2023-01-12T13:49:21Z

@RadekManak this PR is merged now FYI, if you want to pull in also those builders.
#265 (comment)

RadekManak · 2023-01-12T13:53:16Z

Yes, thanks already done. :)

JoelSpeed

Couple of nits, otherwise LGTM

pkg/testutils/resourcebuilder/config/v1/infrastructure.go

pkg/testutils/resourcebuilder/machine/v1beta1/openshift_machine_v1beta1_template.go

elmiko

looks mostly good to me, just a question

elmiko · 2023-01-23T14:58:10Z

Makefile

@@ -69,6 +73,10 @@ vet: ## Apply go vet to all go files
 build-e2e:
 $(DOCKER_CMD) go test -c -o "$(BUILD_DEST)" github.com/openshift/cluster-api-actuator-pkg/pkg/

+.PHONY: unit
+unit: ## Run unit tests


do we need to add a prow configuration to exercise this new target?

Yes, this needs to be set up. The tests do pass locally right now.

Can we get that set up before we merge this? Should be a PR to the release repo to add it right?

I have openshift/release@09ee970 prepared, but we need to merge this first. There is no unit target on master, so it won't pass until this PR adds it.

damdo

Generally LGTM. Thanks for putting this together.
Left mainly a comment regarding importing.

Makefile

go.mod

pkg/testutils/cleanup.go

JoelSpeed

I think I would advise considering the testutils module as if it were a completely independent repository. Set up its own makefile and hack scripts for running the tests, and then link to those targets from the top level using make -C testutils, it will simplify a lot of the bash/makefile logic and keep them better isolated

Makefile

hack/go-vet.sh

hack/test.sh

testutils/resourcebuilder/meta/v1/conditions.go

JoelSpeed · 2023-02-01T11:51:09Z

Makefile

+# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
+ENVTEST_K8S_VERSION = 1.26
+
 PROJECT_DIR := $(shell dirname $(abspath $(lastword $(MAKEFILE_LIST))))
+ENVTEST = go run ${PROJECT_DIR}/vendor/sigs.k8s.io/controller-runtime/tools/setup-envtest


Are these new additions used now we've moved things to a separate makefile?

Forgot to remove this. Thanks

JoelSpeed · 2023-02-01T11:54:57Z

testutils/Makefile

+.PHONY: fmt
+fmt: ## Go fmt your code
+ $(DOCKER_CMD) hack/go-fmt.sh .
+
+.PHONY: goimports
+goimports: ## run goimports
+ $(DOCKER_CMD) hack/goimports.sh .


Are these not covered by GolangCI Lint? My understanding was they are both run as part of its suite

You are correct. I have kept them because, they were being executed by CI. There are currently two targets that run on this repo. (excluding E2Es). check that on main branch runs fmt and vet, and goimports. The golanCI lint has been added recently and is not being executed by CI. If you want, we can remove these and only keep golangci lint and unit.

If that's the case, then yep, lets keep them for now but I think we should migrate to golangci-lint as the single source moving forward, that can be a follow up PR though

JoelSpeed · 2023-02-01T11:57:50Z

go.mod

 k8s.io/klog v1.0.0
 k8s.io/utils v0.0.0-20220823124924-e9cbc92d1a73
 sigs.k8s.io/controller-runtime v0.13.0
+ sigs.k8s.io/controller-runtime/tools/setup-envtest v0.0.0-20221222054525-3c4deba74e5c


Is this correct? I didn't think setup-entest was needed at this level anymore?

Forgot to remove this. Thanks

fmt and goimports are now executed as part of the golangci lint suite

RadekManak · 2023-02-08T10:57:43Z

/retest-required

openshift-ci · 2023-02-08T12:14:37Z

@RadekManak: all tests passed!

Full PR test history. Your PR dashboard.

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.

JoelSpeed · 2023-02-08T17:39:11Z

/approve

/assign @damdo

Dam for LGTM on this as we talked about it during standup

openshift-ci · 2023-02-08T17:45:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

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

damdo

Thanks for putting this together Radek.

/lgtm

openshift-ci bot requested review from damdo and elmiko January 10, 2023 11:45

RadekManak force-pushed the test-utils branch from c35dbf2 to ca770ea Compare January 10, 2023 12:35

RadekManak mentioned this pull request Jan 10, 2023

[OCPCLOUD-1706] Migrate test utils to actuator-pkg repository openshift/cluster-control-plane-machine-set-operator#159

Merged

JoelSpeed reviewed Jan 10, 2023

View reviewed changes

RadekManak force-pushed the test-utils branch from 399c89b to 502e870 Compare January 12, 2023 11:07

JoelSpeed reviewed Jan 12, 2023

View reviewed changes

pkg/testutils/resourcebuilder/config/v1/infrastructure.go Outdated Show resolved Hide resolved

pkg/testutils/resourcebuilder/machine/v1beta1/openshift_machine_v1beta1_template.go Outdated Show resolved Hide resolved

JoelSpeed changed the title ~~Migrate test utils from CPMSO repository~~ [OCPCLOUD-1706] Migrate test utils from CPMSO repository Jan 23, 2023

RadekManak force-pushed the test-utils branch from 4cf2025 to 5308e45 Compare January 23, 2023 11:57

elmiko reviewed Jan 23, 2023

View reviewed changes

damdo reviewed Jan 24, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

go.mod Show resolved Hide resolved

pkg/testutils/cleanup.go Outdated Show resolved Hide resolved

Migrate test utils from CPMSO repository

7180487

RadekManak force-pushed the test-utils branch from 5308e45 to f2ca228 Compare January 27, 2023 13:47

Convert testutils into module

d4cd1b2

RadekManak force-pushed the test-utils branch from f2ca228 to d4cd1b2 Compare January 27, 2023 13:57

JoelSpeed reviewed Jan 27, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

hack/go-vet.sh Outdated Show resolved Hide resolved

hack/test.sh Outdated Show resolved Hide resolved

testutils/resourcebuilder/meta/v1/conditions.go Outdated Show resolved Hide resolved

JoelSpeed reviewed Feb 1, 2023

View reviewed changes

RadekManak added 5 commits February 1, 2023 13:51

Make tooling for testutils independent

a167c3d

Rename StatusCondition to Condition

34fe637

Remove unused setup-envtest

4b534b5

Remove fmt and goimports target

ebafd47

fmt and goimports are now executed as part of the golangci lint suite

Remove unused container setup from testutils Makefile

02f6689

RadekManak force-pushed the test-utils branch from 4923bc3 to 02f6689 Compare February 1, 2023 12:51

openshift-ci bot assigned damdo Feb 8, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2023

damdo reviewed Feb 9, 2023

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2023

openshift-merge-robot merged commit 2ec5c87 into openshift:master Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPCLOUD-1706] Migrate test utils from CPMSO repository #265

[OCPCLOUD-1706] Migrate test utils from CPMSO repository #265

RadekManak commented Jan 10, 2023

damdo commented Jan 10, 2023

JoelSpeed left a comment

JoelSpeed Jan 10, 2023

RadekManak Jan 11, 2023

RadekManak commented Jan 11, 2023 •

edited

Loading

damdo commented Jan 12, 2023 •

edited

Loading

RadekManak commented Jan 12, 2023

JoelSpeed left a comment

elmiko left a comment

elmiko Jan 23, 2023

RadekManak Jan 23, 2023

JoelSpeed Jan 23, 2023

RadekManak Jan 27, 2023

damdo left a comment

JoelSpeed left a comment

JoelSpeed Feb 1, 2023

RadekManak Feb 1, 2023

JoelSpeed Feb 1, 2023

RadekManak Feb 1, 2023

JoelSpeed Feb 2, 2023

JoelSpeed Feb 1, 2023

RadekManak Feb 1, 2023

RadekManak commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

damdo left a comment

[OCPCLOUD-1706] Migrate test utils from CPMSO repository #265

[OCPCLOUD-1706] Migrate test utils from CPMSO repository #265

Conversation

RadekManak commented Jan 10, 2023

damdo commented Jan 10, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RadekManak commented Jan 11, 2023 • edited Loading

damdo commented Jan 12, 2023 • edited Loading

RadekManak commented Jan 12, 2023

JoelSpeed left a comment

Choose a reason for hiding this comment

elmiko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damdo left a comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RadekManak commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

JoelSpeed commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

damdo left a comment

Choose a reason for hiding this comment

RadekManak commented Jan 11, 2023 •

edited

Loading

damdo commented Jan 12, 2023 •

edited

Loading