bump go 1.22 and k8s 1.30 by everettraven · Pull Request #337 · operator-framework/helm-operator-plugins

Comment on lines -86 to -95

		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)
		}
		}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -130 to -131

		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())))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

	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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines -130 to -131

		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())))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

	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.

		// 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")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to return an error instead of boolean

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +284 to +286

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>