Make JaegerSpec.Strategy a type

[pavolloffay]

Strategy is defined as string, it makes it harder to find out what values are correct. It can be defined as a separate type e.g. type DeploymentStrategy string and valid values can be defined as constants.

[volmedo]

I'm planning to give this issue a try :)

[jpkrohling]

Go ahead! If you need any help, let me know.

[volmedo]

What tests should I care about to be sure I'm not breaking anything? Should unit tests be enough or will I need to run full e2e tests?

I just created a type DeployStrategy string and changed JaegerSpec.Strategy from string to DeploymentStrategy and some tests exploded with make test (obviously), but I wonder if getting them to pass again is enough guarantee.

[jpkrohling]

Unit tests should be enough. Then, you can run the operator in a minikube local cluster, deploying the deploy/examples/simplest.yaml to make sure a simple CR can be successfully deployed. If it works fine, ie, the Jaeger pod is created and starts, then there's a good chance that everything works. Send the PR at this point, and the CI will run all the e2e tests, giving us confidence that nothing is broken :-)

[volmedo]

I've created the PR. I used kind instead of minikube, but was able to deploy simplest.yaml successfully.

I also ran unit tests and everything passes, except for TestExtractSecretsToFile_Err and TestWriteToWorkingDir. IMHO, these tests are a bit flaky, since they rely on the error message returned from filesystem IO operations. Such messages are platform-dependent. Should I create an issue for them?

Another concern is that, after implementing my changes, I found this code. Seems something was already implemented. Does this refer to the same strategy "concept"?

[jpkrohling]

I also ran unit tests and everything passes, except for TestExtractSecretsToFile_Err and TestWriteToWorkingDir

Are they passing for you before you apply your changes? If they are failing even before you apply your changes, please create an issue so that we can track the problem.

The code you found is talking about the same strategies, but in a different level. Yours is about the value from the CR, and this is the strategy that is derived from the CR value. You can unify them, removing the existing one, keeping yours. Your change has probably more impact than the existing constant.

[volmedo]

I also ran unit tests and everything passes, except for TestExtractSecretsToFile_Err and TestWriteToWorkingDir

Are they passing for you before you apply your changes? If they are failing even before you apply your changes, please create an issue so that we can track the problem.

They were failing before I changed anything, that's why I think they are flaky. I've already created a new issue for this.

The code you found is talking about the same strategies, but in a different level. Yours is about the value from the CR, and this is the strategy that is derived from the CR value. You can unify them, removing the existing one, keeping yours. Your change has probably more impact than the existing constant.

Great, I've pushed a commit unifying them :)