bump go 1.22 and k8s 1.30 #337

everettraven · 2024-05-16T18:31:07Z

No description provided.

everettraven · 2024-05-16T18:31:38Z

/hold

everettraven · 2024-05-16T20:32:54Z

/hold cancel

everettraven · 2024-05-16T20:34:18Z

internal/cmd/helm-operator/run/cmd.go

-	if f.ManagerConfigPath != "" {
-		// TODO: option to load from config file is deprecated. This will also be removed from here when
-		// componentConfig option is removed.
-		// Refer: https://github.com/kubernetes-sigs/controller-runtime/issues/895
-		cfgLoader := ctrl.ConfigFile().AtPath(f.ManagerConfigPath) //nolint:staticcheck
-		if options, err = options.AndFrom(cfgLoader); err != nil { //nolint:staticcheck
-			log.Error(err, "Unable to load the manager config file")
-			os.Exit(1)
-		}
-	}


The latest controller runtime version removed this configuration method. I removed the flag due to this as per the TODO comment

everettraven · 2024-05-16T20:37:51Z

pkg/reconciler/internal/hook/hook_test.go

-				Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))
-				Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))


Removed because the ctrl.Watch() function signature changed to no longer accept handlers and predicated. They are instead configured via the source.Source type. Instead of creating a custom implementation of the source.Source interface to facilitate this unit test I felt it would be better to remove these assertions as I'm not entirely sure they are that valuable (although I could be wrong, so please let me know if I am!).

It looks like the reason for the existence of these tests is to check whether the correct handler is used. If we are no longer checking the handler specifics, then these tests aren't checking what they say they're checking.

Looks like the option to add additional handler and predicates was to allow passing them during hook_test (

helm-operator-plugins/pkg/reconciler/internal/hook/hook_test.go

Lines 174 to 178 in ae087c4

Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[2].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[3].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

})

). We may face issues with that test, if we remove this.

I'll work on getting this done in a way that we can still have these assertions

Attempted to address this using reflection in 7572d45

I used reflection because this allows us to still utilize the controller-runtime native implementation and test what is intended by these test cases. The only other alternative I could think of is using our own abstraction on top that allows us to capture these values but that felt heavy handed and a more broad impact to the code base for a unit test specific need. I'm not set in stone on using reflection and am open to other suggestions.

pkg/reconciler/reconciler.go

codecov-commenter · 2024-05-16T20:50:47Z

Codecov Report

Attention: Patch coverage is 75.51020% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 48.35%. Comparing base (08ab7fb) to head (6f03f8b).
Report is 19 commits behind head on main.

Files	Patch %	Lines
pkg/internal/predicate/predicates.go	18.18%	8 Missing and 1 partial ⚠️
pkg/reconciler/internal/hook/hook.go	89.47%	0 Missing and 2 partials ⚠️
internal/flags/flag.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #337       +/-   ##
===========================================
- Coverage   85.06%   48.35%   -36.72%     
===========================================
  Files          19       62       +43     
  Lines        1346     2914     +1568     
===========================================
+ Hits         1145     1409      +264     
- Misses        125     1423     +1298     
- Partials       76       82        +6

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

rashmigottipati · 2024-05-16T20:58:19Z

Do we need to bump the go version for the testdata as well?

everettraven · 2024-05-16T21:00:38Z

Do we need to bump the go version for the testdata as well?

I think to properly bump this - yes. I think this means we have to wait for Kubebuilder to bump the support in their Go plugin, cut a release, and pull that in. I'm not sure we should block on that though since for the OLMv1 repositories we don't need any of the Kubebuilder based stuff.

I will consult with @varshaprasad96 and @joelanford tomorrow morning to get their thoughts

everettraven · 2024-05-17T13:46:03Z

@rashmigottipati I realized that the generated testdata is of the hybrid plugin and we control the versions there. Should be updated as of a09a349

pkg/plugins/hybrid/v1alpha/scaffolds/init.go

joelanford

The removal of the handler checks in hook_test.go are a blocker for me. We need to find a way to make sure the correct handlers are being used based on the relationship between the owner and ownee objects.

varshaprasad96 · 2024-05-17T15:30:19Z

pkg/reconciler/internal/hook/hook_test.go

-				Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))
-				Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))


Looks like the option to add additional handler and predicates was to allow passing them during hook_test (

helm-operator-plugins/pkg/reconciler/internal/hook/hook_test.go

Lines 174 to 178 in ae087c4

Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[2].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

Expect(c.WatchCalls[3].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))

})

). We may face issues with that test, if we remove this.

pkg/plugins/hybrid/v1alpha/scaffolds/init.go

joelanford · 2024-05-20T13:59:13Z

pkg/reconciler/internal/hook/hook_test.go

+// It is assumed that the source.Source was created via the source.Kind() function.
+func validateSourceHandlerType(s source.Source, expected interface{}) bool {
+	sourceVal := reflect.Indirect(reflect.ValueOf(s))
+	handlerFieldVal := sourceVal.FieldByName("Handler")


Is it possible for this to return nil?

If so, we should do nil checks, and perhaps change the function signature to return error instead of bool such that we can return some meaningful information when the validation fails.

I don't believe it will return the literal value nil, but there is a value.IsNil() method on the returned type.

I do think returning an error instead of a boolean is a good idea. Will update now

Updated to return an error instead of boolean

What if there is no Handler field? Seems like we should have an explicit error for that. That way if the underlying source implementation changes, we have a pretty good signal to go fix our test code.

I've updated this to use the value.IsValid() function to validate that the returned value is valid and we can perform validations with it. Also added some simple tests to ensure that it is functioning as expected

pkg/reconciler/internal/hook/hook_test.go

everettraven · 2024-05-20T15:56:01Z

pkg/reconciler/internal/hook/hook_test.go

+	if !sourceVal.IsValid() {
+		return errors.New("provided source.Source value is invalid")
+	}


Just to note, reading the reflect package documentation it seemed like all other functions would panic on an invalid value and the IsValid() function is the way to check for validity. As far as I am aware, this doesn't give us any additional information other than that the value is invalid

Signed-off-by: everettraven <everettraven@gmail.com>

openshift-ci bot requested review from camilamacedo86 and fabianvf May 16, 2024 18:31

github-actions bot added area/deps area/sdk area/testing area/main-binary labels May 16, 2024

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

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

everettraven requested a review from rashmigottipati May 16, 2024 20:33

everettraven commented May 16, 2024

View reviewed changes

pkg/reconciler/reconciler.go Outdated Show resolved Hide resolved

everettraven force-pushed the chore/bump-k8s-1.30 branch from ae2a504 to f5caf4a Compare May 16, 2024 20:55

joelanford reviewed May 17, 2024

View reviewed changes

pkg/plugins/hybrid/v1alpha/scaffolds/init.go Outdated Show resolved Hide resolved

joelanford requested changes May 17, 2024

View reviewed changes

varshaprasad96 reviewed May 17, 2024

View reviewed changes

joelanford reviewed May 20, 2024

View reviewed changes

pkg/reconciler/internal/hook/hook_test.go Outdated Show resolved Hide resolved

everettraven mentioned this pull request May 20, 2024

✨ Modify ClusterExtension to use Helm under the hood to apply contents into the cluster operator-framework/operator-controller#846

Merged

4 tasks

everettraven commented May 20, 2024

View reviewed changes

joelanford approved these changes May 20, 2024

View reviewed changes

bump go 1.22 and k8s 1.30

6f03f8b

Signed-off-by: everettraven <everettraven@gmail.com>

everettraven force-pushed the chore/bump-k8s-1.30 branch from 8752ba7 to 6f03f8b Compare May 20, 2024 17:59

everettraven enabled auto-merge May 20, 2024 18:01

everettraven added this pull request to the merge queue May 20, 2024

Merged via the queue into operator-framework:main with commit f463c36 May 20, 2024
5 of 6 checks passed

everettraven deleted the chore/bump-k8s-1.30 branch May 20, 2024 18:14

Neo2308 mentioned this pull request May 22, 2024

Meta Issue for k8s 1.30 bump operator-framework/operator-sdk#6730

Open

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bump go 1.22 and k8s 1.30 #337

bump go 1.22 and k8s 1.30 #337

everettraven commented May 16, 2024

everettraven commented May 16, 2024

everettraven commented May 16, 2024

everettraven May 16, 2024

everettraven May 16, 2024

joelanford May 17, 2024

varshaprasad96 May 17, 2024

everettraven May 17, 2024

everettraven May 20, 2024

codecov-commenter commented May 16, 2024 •

edited

rashmigottipati commented May 16, 2024

everettraven commented May 16, 2024 •

edited

everettraven commented May 17, 2024

joelanford left a comment

varshaprasad96 May 17, 2024

joelanford May 20, 2024

everettraven May 20, 2024

everettraven May 20, 2024

joelanford May 20, 2024 •

edited

everettraven May 20, 2024 •

edited

everettraven May 20, 2024

		Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))
		Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(handler.EnqueueRequestForOwner(sch, rm, owner, handler.OnlyControllerOwner())))

	Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
	Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
	Expect(c.WatchCalls[2].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
	Expect(c.WatchCalls[3].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{}))
	})

bump go 1.22 and k8s 1.30 #337

bump go 1.22 and k8s 1.30 #337

Conversation

everettraven commented May 16, 2024

everettraven commented May 16, 2024

everettraven commented May 16, 2024

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

codecov-commenter commented May 16, 2024 • edited

Codecov Report

rashmigottipati commented May 16, 2024

everettraven commented May 16, 2024 • edited

everettraven commented May 17, 2024

joelanford 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

joelanford May 20, 2024 • edited

Choose a reason for hiding this comment

everettraven May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 16, 2024 •

edited

everettraven commented May 16, 2024 •

edited

joelanford May 20, 2024 •

edited

everettraven May 20, 2024 •

edited