Make it easy to run a single test

[raghavendra-talur]

Describe what this PR does

This PR moves the config init code to the main function from the global init. Doing so, it allows for such config init to be skipped for tests or other scenarios.

In the second commit, we set the os environment variable for the operator namespace in the BeforeSuite() function if it is not already set. This allows users to run a specific test by using the ginkgo cli or by using the test decorators in vscode.

For example:

$ ginkgo -focus "ClientProfile Controller" -r
[1732083376] Controller Suite - 1/3 specs SS• SUCCESS! 5.797831s PASS
  Starting ceph-csi-operator suite
[1732083376] e2e suite - 0/1 specs S SUCCESS! 59.25µs PASS

Ginkgo ran 2 suites in 10.605577042s
Test Suite Passed

Provide some context for the reviewer

Is there anything that requires special attention

Earlier, the configuration was automatically inited. Now, it is being done manually in the main function.

Is the change backward compatible?
Yes

Are there concerns around backward compatibility?
No

Checklist:

• Commit Message Formatting: Commit titles and messages follow
  guidelines in the developer
  guide.
• Reviewed the developer guide on Submitting a Pull
  Request
• Pending release
  notes
  updated with breaking and/or notable changes for the next major release.
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.

[leelavg]

Isn't only second commit is enough for the required behavior, assuming the behavior is to have operator namespace to be set? First commit just delays the initialization w/o any functionality change, isn't it?

[raghavendra-talur]

Isn't only second commit is enough for the required behavior, assuming the behavior is to have operator namespace to be set? First commit just delays the initialization w/o any functionality change, isn't it?

Good question. If I run the test with just the commit that sets the env var then I see this error:

$ ginkgo -focus "ClientProfile Controller" -r
panic: Required OPERATOR_NAMESPACE environment variable is either missing or empty

goroutine 1 [running]:
github.com/ceph/ceph-csi-operator/internal/controller.init.func1(...)
        /Users/rtalur/src/github.com/raghavendra-talur/ceph-csi-operator/internal/controller/defaults.go:71
github.com/ceph/ceph-csi-operator/internal/utils.Call[...](...)
        /Users/rtalur/src/github.com/raghavendra-talur/ceph-csi-operator/internal/utils/core.go:121
github.com/ceph/ceph-csi-operator/internal/controller.init()
        /Users/rtalur/src/github.com/raghavendra-talur/ceph-csi-operator/internal/controller/defaults.go:68 +0x3c8

Ginkgo ran 2 suites in 6.842060084s

There were failures detected in the following suites:
  controller ./internal/controller

Test Suite Failed

The reason is that these variables were being assigned at the global scope by calling a function.

var operatorNamespace = utils.Call(func() string {
        namespace := os.Getenv("OPERATOR_NAMESPACE")
        if namespace == "" {
                panic("Required OPERATOR_NAMESPACE environment variable is either missing or empty")
        }
        return namespace
})

This makes these function calls part of the Go Init() process. The setting of env var in suite_test.go would be run after the Init() process but that would be too late as this function panics on not finding the env var set.

[Madhu-1]

in the AfterSuit lets unset this env if we are setting it

[raghavendra-talur]

Done

[leelavg]

Isn't the walrus shawdowing global operatorNamespace var here? Similar for others as well.

[raghavendra-talur]

Fixed, thanks!

[leelavg]

Possible to hold off the merge till tomorrow? pls excuse I missed checking this today.

The approach isn't sitting well w/ me for a couple of reasons, BTW open to be corrected.

1. We are creating an indirect dependency on the pkg that something MUST be done in addition to importing it for controllers to work
2. Amending a fundamental requirement to be able to run test, kinda inverted.

[raghavendra-talur]

Possible to hold off the merge till tomorrow? pls excuse I missed checking this today.

The approach isn't sitting well w/ me for a couple of reasons, BTW open to be corrected.

1. We are creating an indirect dependency on the pkg that something MUST be done in addition to importing it for controllers to work
2. Amending a fundamental requirement to be able to run test, kinda inverted.

Yes, I am in no hurry to get this merged. I will reply with examples to answer the above questions.

[leelavg]

Looked again, the fix introduces a bug in both test & runtime environments which I already left a comment.

I couldn't find any simpler alternatives, I don't use the feature (vscode & ginkgo) which requires this fix and leave the decision to maintainers.

BTW, see if t.Setenv helps in anyway which removes explicit cleanup.

[raghavendra-talur]

The approach isn't sitting well w/ me for a couple of reasons, BTW open to be corrected.

1. We are creating an indirect dependency on the pkg that something MUST be done in addition to importing it for controllers to work

This is not true. Just importing the controllers package in main.go wasn't enough even before. The SetupWithManager() has to be invoked for every controller in the package, which are 3 at this time, ClientProfileMappingReconciler, ClientProfileReconciler, and DriverReconciler.

2. Amending a fundamental requirement to be able to run test, kinda inverted.

I don't understand which requirement is amended here?

[raghavendra-talur]

Ok, the tests have passed but now I agree with @leelavg that it is starting to look like a big change for a project which I am not the maintainer.

@nb-ohad @Madhu-1 I will leave it up to you to merge or close the PR.

Ginkgo and VSCode both provide users a way to specify an env var file and that is one alternative option for the users.

[leelavg]

Just importing the controllers package in main.go wasn't enough even before.

• let me extend what I meant. Making it mandatory to run controllers.Init() for a side effect isn't exactly same as setting up controller. This creates the coupling b/n all controllers and this init as this is manipulating a var at global scope. It's kinda fine atm since this is done before setup of controllers as they can ask for concurrent reconciles.
• with existing code one can choose to not setup a controller at all but always forced to do controller.Init()

which requirement is amended here?

• I mean, we usually test what is expected to run and here we are changing runtime requirements to be able to test it unlike the usage of stubs, fakes, doubles, mocks etc (I'm not emphasizing on these constructs but only pointing methodologies)

---

having said that, I acknowledge the usage of global vars which might be used in more than one place is problematic in existing code and I feel proposed fix isn't easing it much.

[Madhu-1]

Ginkgo and VSCode both provide users a way to specify an env var file and that is one alternative option for the users.

@raghavendra-talur we are reworking on the new framework for the testing, Lets keep this PR open for sometime and we can see if we can get this in or we need any additional changes later on.