Dennis-Mircea commented on code in PR #1144:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1144#discussion_r3511349709
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/validation/DefaultValidator.java:
##########
@@ -154,7 +154,9 @@ private Optional<String>
validateFlinkVersion(FlinkDeployment deployment) {
var spec = deployment.getSpec();
var version = spec.getFlinkVersion();
if (version == null) {
- return Optional.of("Flink Version must be defined.");
+ // Presence is enforced by the CRD schema (@Required on
+ // FlinkDeploymentSpec.flinkVersion); nothing further to validate
here.
+ return Optional.empty();
Review Comment:
The new validation is admission-only and does not cover the reconcile cycle.
The new added constraints are enforced only at admission, and only if the
updated CRD is installed, which is a manual step (Helm does not auto-upgrade
`crds/`). `DefaultValidator`, however, runs on every reconcile as well, so
removing these checks drops the runtime validation checks: after an operator
bump where the CRD has not been replaced yet, an invalid spec reaching
reconcile is no longer caught and hits deploy logic with a bad value.
##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/spec/FlinkStateSnapshotSpec.java:
##########
@@ -46,6 +47,7 @@ public class FlinkStateSnapshotSpec {
* Maximum number of retries before the snapshot is considered as failed.
Set to -1 for
* unlimited or 0 for no retries.
*/
+ @Min(-1)
Review Comment:
This doesn't look as a necessary validation constraint.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
