strimzi-kafka-operator
strimzi-kafka-operator copied to clipboard
strimzi
[Enhancement]: Create enum for custom resource Condition types
Related problem
At this time, we hard-code the value of the type of Conditions in the status of custom resources across the codebase, checkout the usage of the these two Condition type Strings:
- "Warning" [1]
- "Ready" [2]
Suggested solution
This task would entail creating a "ConditionType" enum with the possible Condition types e.g:
public enum ConditionType {
WARNING("Warning"),
ERROR("Error"),
READY("Ready");
...
}
and referencing this enum type throughout the codebase instead of using the hardcoded String values.
We would also need to consider how this enum would interact with classes like io.strimzi.api.kafka.model.common.CustomResourceConditions [3]
Alternatives
No response
Additional context
[1] https://github.com/search?q=repo%3Astrimzi%2Fstrimzi-kafka-operator+%5C%22Warning%5C%22&type=code [2] https://github.com/search?q=repo%3Astrimzi%2Fstrimzi-kafka-operator+%5C%22Ready%5C%22+&type=code [3] https://github.com/strimzi/strimzi-kafka-operator/blob/main/api/src/main/java/io/strimzi/api/kafka/model/common/CustomResourceConditions.java
You should be careful with this as we in general, want to mirror the Kube API and that is IMHO a free string without any restrictions. So I do not think this should touch the API part of the codebase.
Triaged on 29.5.2025: We can implement this, but we should not touch the API part of Strimzi codebase (as Jakub mentioned).
@kyguy @im-konge I'd like to take this up if it still needs to be completed?
It's all yours if you want it @OwenCorrigan76, feel free to assign it to yourself.
Curious to see the PR to understand what sense does it make. IMHO it will just make the code harder to read and too complicated.
Supporting Jakub I don't remember where but ... I remember that we decided to keep the strings because this is how Kubernetes works. You could put whatever you want there. Maybe we should just close this issue?
we in general, want to mirror the Kube API and that is IMHO a free string without any restrictions.
If there's a specific reason we or the Kubernetes team handle it this way, I'm happy to leave it as is and close the issue. It just struck me as odd to see these values hardcoded throughout the codebase, so I raised it in case there was something I was missing.
In any case, I’ll defer to the maintainers on whether this is worth pursuing. I’d rather not take up @OwenCorrigan76’s time if there’s no clear benefit to making a change here.
we in general, want to mirror the Kube API and that is IMHO a free string without any restrictions.
If there's a specific reason we or the Kubernetes team handle it this way, I'm happy to leave it as is and close the issue. It just struck me as odd to see these values hardcoded throughout the codebase, so I raised it in case there was something I was missing.
I'm not sure what exactly you mean by hardcoded here. If there are Strings (as in "Ready") all over the code base, it does not necessarily mean Enum is the right solution. But you can - for example - create some class ConditionTypes and have the types defined there as String constants and used through the code base. Enum would not make much sense to me if you have to keep decoding and encoding it between the API and other classes.
I'm not sure what exactly you mean by hardcoded here.
What I mean by "hardcoded" is that condition types like "Ready" and "Warning" are written directly as literal String values in the code, rather than being defined as constants or enums.
it does not necessarily mean Enum is the right solution. But you can - for example - create some class ConditionTypes and have the types defined there as String constants and used through the code base. Enum would not make much sense to me if you have to keep decoding and encoding it between the API and other classes.
You're right - using an enum isn't necessarily the best solution here. In hindsight, I shouldn't have included it in the issue title; it was meant more as a suggestion for how we might address the problem. Creating a class with String constants as an alternative solution sounds reasonable to me.
By using a class exposing Strings we are losing the type-safety because anyway we can pass any String (instead of the ones coming frm the ConditionType class for example). It doesn't mean I am totally against it but ...
... if you have to keep decoding and encoding it between the API and other classes.
@scholzj can elaborate what are you referring to?
I looked through the code - searching for "Warning" and "Ready". I do not see any excesive use of it in the code. Sure, there are many tests checking the condition. But it is their point to check the right value there. So I do not think you should really replace that with anything. In the production code, it seems to be in few places only. So I'm not sure there is really much to do here one way or another to be honest.
@ppatierno If we respect the Kubernetes API and backwards compatibility of our API and keep using Strings in our api module. That means that the Enum would need to be created everytime you read from the API classes or write to them. Take this example method from StatusUtils:
public static Condition buildWarningCondition(String reason, String message, String transitionTime) {
return new ConditionBuilder()
.withLastTransitionTime(transitionTime)
.withType("Warning")
.withStatus("True")
.withReason(reason)
.withMessage(message)
.build();
}
Are you really going to change it to something like this?
public static Condition buildWarningCondition(String reason, String message, String transitionTime) {
return new ConditionBuilder()
.withLastTransitionTime(transitionTime)
.withType(ConditionTypeEnum.WARNING.toString())
.withStatus("True")
.withReason(reason)
.withMessage(message)
.build();
}
That does not seem to make much sense unless you would use the enum extensively in the other parts of the code.
What I proposed - having a class with String constants ... honestly, I can probably live without it. But it would at least centralize all the places similarly to the list of labels and annotations in their respective classes.
But as I said, looking through the code, I do not see this as an issue and would probably close this. But maybe I missed some part of the code where @kyguy perceived some problems.
It would have to be something like ConditionTypeEnum.WARNING.toString() if refactored.
@kyguy Do you still think this is worth persuing, given the comments above?
I do not see any excesive use of it in the code. Sure, there are many tests checking the condition. But it is their point to check the right value there. So I do not think you should really replace that with anything. In the production code, it seems to be in few places only. So I'm not sure there is really much to do here one way or another to be honest.
If we exclude the test classes, there aren't that many usages, around ~5 classes of production code. I ran the following command to display the usages in the production code.
grep -rE '"(Warning|Ready|Error)"' . \
--exclude-dir='*Test*' \
--exclude-dir='*test*' \
--exclude-dir='*packaging*' \
--exclude-dir='*examples*' \
--exclude-dir='*api*' \
--exclude='*Test*' \
--exclude='*test*' \
--exclude='*packaging*' \
--exclude='*examples*'
If it doesn't make sense to update the test classes and the limited production class usages don't merit a String constant class, I’m fine to close the issue.
It would have to be something like ConditionTypeEnum.WARNING.toString() if refactored.
The alternative solution suggested in the comments above is to use a class with String constants instead of an Enum. e.g.
public class ConditionType {
public static final String WARNING = "Warning";
public static final String READY = "Ready";
public static final String ERROR = "Error";
...
}
But as I said, looking through the code, I do not see this as an issue and would probably close this.
Do you still think this is worth persuing, given the comments above?
I’ll defer to the maintainers, if limited production code usage doesn’t justify the change, I’m fine with closing the issue for now.
I'm happy for this to be closed for the following reasons:
- Apart from in tests this isn't used very often so the benefit seems small
- In the ConditionBuilder fluent API the type must be passed as a String, making having an enum less attractive
- Builder classes are generated so we can't change them to for example have a
.withWarning()method
Triaged on 17/7/25: closing as we don't think this is necessary at this time, but in future can revisit if more instances of the Strings are added