search

quarkiverse / quarkus-operator-sdk

Quarkus Extension to create Kubernetes Operators in Java using the Java Operator SDK (https://github.com/java-operator-sdk/java-operator-sdk) project
Apache License 2.0
119 stars 50 forks source link

Bundle generator should provide a valid default name if application name is not provided #313

Open metacosm opened 2 years ago

Sgitario commented 2 years ago

As far I know, the application name can't be empty (or not provided). By default, it's the name of the module.

metacosm commented 2 years ago

It's empty during tests at the very least, if you don't explicitly set it, which is only possible, as far as I'm aware, if you're using QuarkusProdModeTest.

Sgitario commented 2 years ago

It's empty during tests at the very least, if you don't explicitly set it, which is only possible, as far as I'm aware, if you're using QuarkusProdModeTest.

Yes, with QuarkusProdModeTest, it's using "<< unset >>" or similar.

metacosm commented 2 years ago

Yes but that means that, when not using QuarkusProdModeTest, the tests will generate invalid manifests.

Sgitario commented 2 years ago

I don't follow you: when testing this, the application name is << unset >>, but when doing a real build, it will use the module name.

metacosm commented 2 years ago

I don't follow you: when testing this, the application name is << unset >>, but when doing a real build, it will use the module name.

<< unset >> is not a valid Kubernetes name. When testing, the generated resources will use that invalid name and therefore be invalid themselves. Generated resources should be as close as possible to what should be generated at runtime and we should make it so that our code generates valid resources under all foreseeable circumstances.

Sgitario commented 2 years ago

I don't follow you: when testing this, the application name is << unset >>, but when doing a real build, it will use the module name.

<< unset >> is not a valid Kubernetes name. When testing, the generated resources will use that invalid name and therefore be invalid themselves. Generated resources should be as close as possible to what should be generated at runtime and we should make it so that our code generates valid resources under all foreseeable circumstances.

But this is never going to be a real issue. Also, if you don't want to have the << unset >> value, simply set one using the method QuarkusProdModeTest.setApplicationName like in https://github.com/quarkiverse/quarkus-operator-sdk/blob/6664a5b280bcb5665742922ff8a0d8b3ff8b0bb8/bundle-generator/deployment/src/test/java/io/quarkiverse/operatorsdk/bundle/DefaultBundleWhenNoCsvMetadataTest.java#L21

metacosm commented 2 years ago

You cannot set the application name if you're using QuarkusTest only.

metacosm commented 2 years ago

I agree that it shouldn't cause an issue in most cases. However, this shows that there are at least 2 issues with the code that deals with this:

  1. improper input validation
  2. improper default value in the absence of valid input value (note that the default "value" in this case might very well be throwing an IllegalArgumentException or something similar)

This is the basis of the Robustness Principle. Whatever the input is, the output should be conform to expectations (in this case, valid manifests).