Refactor some things #2775

2uasimojo · 2025-10-31T22:50:16Z

What started this was HIVE-2302 / #2729 where I wanted to be able to use the ConfigureCreds map from installmanager as well as deprovision, but couldn't do it easily due to circular imports. I got that done, but also knocked down some other tech debt at the same time. I moved things closer to where they're used, in many cases eliminating the packages they came from, addressed some deprecations, deleted some unused stuff, shaved off a few lines of code in some places, got rid of some soft linting squiggles, DRYed some duplicated symbols, that kind of thing.

2uasimojo · 2025-10-31T22:50:48Z

NOTE: Currently on top of #2729. Review top commit only.

2uasimojo · 2025-11-03T15:57:44Z

/retest

red-hat-konflux · 2025-11-03T15:57:55Z

Caution

There are some errors in your PipelineRun template.

PipelineRun	Error
hive-mce-210-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`
hive-mce-26-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`
hive-mce-27-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`
hive-mce-28-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`
hive-mce-29-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`
hive-on-pull-request	`CEL expression evaluation error: expression "event == \"pull_request\"\n&& !body.pull_request.draft\n&& target_branch == \"master\"\n&& !files.all.all(x, x.matches('^docs/\|\\\\.md$\|^(?:.*/)?(?:\\\\.gitignore\|OWNERS\|PROJECT\|LICENSE)$'))\n" failed to evaluate: no such key: pull_request`

codecov · 2025-11-03T16:40:04Z

Codecov Report

❌ Patch coverage is 12.04819% with 365 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.34%. Comparing base (161a023) to head (9ccaa4c).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
contrib/pkg/awsprivatelink/endpointvpc/helpers.go	0.00%	113 Missing ⚠️
pkg/creds/aws/aws.go	0.00%	41 Missing ⚠️
contrib/pkg/createcluster/create.go	0.00%	30 Missing ⚠️
pkg/operator/hive/hive_controller.go	0.00%	21 Missing ⚠️
pkg/resource/helper.go	0.00%	20 Missing ⚠️
contrib/pkg/adm/managedns/enable.go	0.00%	13 Missing ⚠️
pkg/operator/hive/apply.go	0.00%	13 Missing ⚠️
contrib/pkg/clusterpool/clusterpool.go	0.00%	12 Missing ⚠️
pkg/operator/hive/hiveadmission.go	0.00%	11 Missing ⚠️
contrib/pkg/awsprivatelink/endpointvpc/remove.go	0.00%	10 Missing ⚠️
... and 33 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2775      +/-   ##
==========================================
+ Coverage   50.19%   50.34%   +0.14%     
==========================================
  Files         284      279       -5     
  Lines       34267    34167     -100     
==========================================
+ Hits        17200    17201       +1     
+ Misses      15713    15612     -101     
  Partials     1354     1354

Files with missing lines	Coverage Δ
cmd/manager/main.go	`0.00% <ø> (ø)`
contrib/pkg/awsprivatelink/awsprivatelink.go	`0.00% <ø> (ø)`
pkg/clusterresource/builder.go	`63.02% <100.00%> (ø)`
pkg/constants/constants.go	`100.00% <ø> (ø)`
...g/controller/clusterpool/clusterpool_controller.go	`59.05% <100.00%> (ø)`
pkg/controller/clustersync/templates.go	`76.00% <100.00%> (ø)`
pkg/controller/images/controller_images.go	`77.77% <ø> (+14.14%)`	⬆️
pkg/controller/machinepool/gcpactuator.go	`71.22% <100.00%> (ø)`
...g/controller/machinepool/machinepool_controller.go	`63.40% <100.00%> (ø)`
pkg/controller/utils/clientwrapper.go	`42.85% <100.00%> (+0.75%)`	⬆️
... and 65 more

🚀 New features to boost your workflow:

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

2uasimojo · 2025-11-03T18:17:48Z

/test e2e-azure

infra flake

2uasimojo · 2025-11-03T20:08:44Z

/hold because

NOTE: Currently on top of #2729.

2uasimojo

notes for reviewers

2uasimojo · 2025-11-04T21:27:05Z

cmd/manager/main.go

 		log.WithField("pprof_host_port", pprofHostPort).Info("Enabling pprof")
 		log.Println(http.ListenAndServe(pprofHostPort, nil))
 	}()
-	rand.Seed(time.Now().UnixNano())


deprecated/obsolete/no-op

It was interesting to learn why this was done in the first place - glad it is no longer needed.

2uasimojo · 2025-11-04T21:27:25Z

contrib/pkg/adm/managedns/enable.go

-	cloudAWS                = "aws"
-	cloudGCP                = "gcp"
-	cloudAzure              = "azure"


use global consts

2uasimojo · 2025-11-04T21:28:00Z

contrib/pkg/adm/managedns/enable.go

 	// create a default one once run.
 	hc := &hivev1.HiveConfig{}
-	o.hiveClient.Get(context.TODO(), types.NamespacedName{Name: hiveConfigName}, hc)
-	if err != nil {


this was checking the wrong err (which couldn't be nil if we got here).

2uasimojo · 2025-11-04T21:29:40Z

contrib/pkg/awsprivatelink/awsprivatelink.go

+const (
+	// privateLinkHubAcctCredsName is the name of the AWS PrivateLink Hub account credentials Secret
+	// created by the "hiveutil awsprivatelink enable" command
+	privateLinkHubAcctCredsName = "awsprivatelink-hub-acct-creds"
+
+	// privateLinkHubAcctCredsLabel is added to the AWS PrivateLink Hub account credentials Secret
+	// created by the "hiveutil awsprivatelink enable" command and
+	// referenced by HiveConfig.spec.awsPrivateLink.credentialsSecretRef.
+	privateLinkHubAcctCredsLabel = "hive.openshift.io/awsprivatelink-hub-acct-credentials"
+)


only used in this package, so moved here from github.com/openshift/hive/contrib/pkg/utils/aws

2uasimojo · 2025-11-04T21:30:47Z

contrib/pkg/awsprivatelink/endpointvpc/helpers.go

@@ -0,0 +1,192 @@
+package endpointvpc


All of these only used in this package, so moved from #2775 and unexported. Should be direct cut/paste (other than capitalization).

I think you provided the wrong link here.
Note to self: moved from contrib/pkg/utils/aws

2uasimojo · 2025-11-04T22:16:10Z

test/e2e/common/deployment.go

-		&appsv1.Deployment{},
-		0,
-		cache.ResourceEventHandlerFuncs{
+	_, controller := cache.NewInformerWithOptions(cache.InformerOptions{


2uasimojo · 2025-11-04T22:17:47Z

test/e2e/common/service.go

-		&corev1.Service{},
-		0,
-		cache.ResourceEventHandlerFuncs{
+	_, controller := cache.NewInformerWithOptions(cache.InformerOptions{


2uasimojo · 2025-11-04T22:18:13Z

test/e2e/destroycluster/destroy_test.go

 	return nil
 }
-
-func failTestFunc(t *testing.T, logger *log.Entry) func(string, ...interface{}) {


collapse single use func to inline

2uasimojo · 2025-11-04T22:19:03Z

test/e2e/postinstall/machinesets/infra_test.go

 		default:
 			t.Log("found cluster autoscaler")
-			break
+			break poll


linter be like, I don't think this was doing what you think it was doing.

2uasimojo · 2025-11-04T22:19:45Z

test/e2e/postinstall/machinesets/infra_test.go

 	clusterAutoscaler := &autoscalingv1.ClusterAutoscaler{}
-	for i := 0; i < 10; i++ {
+poll:
+	for range 10 {


soft lint squiggle

2uasimojo · 2025-11-05T14:51:58Z

/assign @suhanime

suhanime

Nice set of changes! I only have 1 comment you might want to address - and can also squash the second commit, rest LGTM

suhanime · 2025-11-05T19:14:12Z

cmd/manager/main.go

 		log.WithField("pprof_host_port", pprofHostPort).Info("Enabling pprof")
 		log.Println(http.ListenAndServe(pprofHostPort, nil))
 	}()
-	rand.Seed(time.Now().UnixNano())


It was interesting to learn why this was done in the first place - glad it is no longer needed.

suhanime · 2025-11-05T19:35:31Z

contrib/pkg/awsprivatelink/endpointvpc/helpers.go

@@ -0,0 +1,192 @@
+package endpointvpc


I think you provided the wrong link here.
Note to self: moved from contrib/pkg/utils/aws

suhanime · 2025-11-06T07:32:00Z

pkg/controller/machinepool/ibmcloudactuator_test.go

 }

-func mockGetVPCZonesForRegion(ibmClient *mockibm.MockAPI, zones []string, region string) {
+func mockGetVPCZonesForRegion(ibmClient *mockibm.MockAPI, zones []string) {


A true mock for GetVPCZonesForRegion should be using the arg region instead of hardcoded testRegion but given that we always called it with testRegion anyways, I guess this is alright.

suhanime · 2025-11-06T07:44:38Z

pkg/controller/utils/nutanixutils/nutanix.go

The changes in this file might need to be revisited. At the bare minimum, if these args aren't needed, all the relevant docstrings need to be updated

They're not args; they're type declarations for the generics. They were redundant because go can infer them based on the types of the params/returns.

suhanime · 2025-11-06T08:46:55Z

pkg/operator/hive/hiveadmission.go

-
-		wh := requiredObj.(*admregv1.ValidatingWebhookConfiguration)
-		validatingWebhooks[i] = wh
+		validatingWebhooks[i] = readRuntimeObjectOrDie[*admregv1.ValidatingWebhookConfiguration](


I was going to comment for the same in the previous commit, glad you caught it

What started this was HIVE-2302 / openshift#2729 where I wanted to be able to use the ConfigureCreds map from installmanager as well as deprovision, but couldn't do it easily due to circular imports. I got that done, but also knocked down some other tech debt at the same time. I moved things closer to where they're used, in many cases eliminating the packages they came from, addressed some deprecations, deleted some unused stuff, shaved off a few lines of code in some places, got rid of some soft linting squiggles, DRYed some duplicated symbols, that kind of thing.

suhanime · 2025-11-06T19:45:23Z

/lgtm

openshift-ci · 2025-11-06T19:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, suhanime

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~~ [2uasimojo,suhanime]

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

openshift-ci-robot · 2025-11-06T20:30:24Z

/retest-required

Remaining retests: 0 against base HEAD 161a023 and 2 for PR HEAD 9ccaa4c in total

openshift-ci · 2025-11-06T23:35:46Z

@2uasimojo: 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-sigs/prow repository. I understand the commands that are listed here.

In openshift#2775 we rearranged the way installmanager figures out which platform-specific ConfigureCreds() function to call, but forgot to include IBMCloud in the lookup table. (BareMetal is there, which we don't use, and which is probably why this was missed, as it made the number of entries correct.)

openshift-ci bot requested a review from jstuever October 31, 2025 22:50

openshift-ci bot requested a review from suhanime October 31, 2025 22:50

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 31, 2025

2uasimojo force-pushed the le-big-refactor branch from 3c3fddd to 25ce097 Compare October 31, 2025 23:06

2uasimojo force-pushed the le-big-refactor branch from 25ce097 to 975ea68 Compare November 3, 2025 17:03

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2025

2uasimojo force-pushed the le-big-refactor branch from 975ea68 to 8aa2c15 Compare November 4, 2025 20:43

2uasimojo commented Nov 4, 2025

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2025

openshift-ci bot assigned suhanime Nov 5, 2025

suhanime reviewed Nov 6, 2025

View reviewed changes

2uasimojo force-pushed the le-big-refactor branch from a7645be to 9ccaa4c Compare November 6, 2025 19:14

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 6, 2025

openshift-merge-bot bot merged commit d12d5f1 into openshift:master Nov 6, 2025
26 checks passed

2uasimojo deleted the le-big-refactor branch November 7, 2025 18:11

2uasimojo mentioned this pull request Nov 18, 2025

OCPBUGS-65681: Oops, I forgot IBMCloud #2786

Merged

Refactor some things #2775

Refactor some things #2775

Uh oh!

Conversation

2uasimojo commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

2uasimojo commented Oct 31, 2025

Uh oh!

2uasimojo commented Nov 3, 2025

Uh oh!

red-hat-konflux bot commented Nov 3, 2025

Uh oh!

codecov bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

2uasimojo commented Nov 3, 2025

Uh oh!

2uasimojo commented Nov 3, 2025

Uh oh!

2uasimojo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

2uasimojo commented Nov 5, 2025

Uh oh!

suhanime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suhanime commented Nov 6, 2025

Uh oh!

openshift-ci bot commented Nov 6, 2025

Uh oh!

openshift-ci-robot commented Nov 6, 2025

Uh oh!

openshift-ci bot commented Nov 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

2uasimojo commented Oct 31, 2025 •

edited

Loading

codecov bot commented Nov 3, 2025 •

edited

Loading