Switch operator to code-first#5038
Conversation
| package io.apicurio.registry.operator; | ||
|
|
||
| import io.apicur.registry.v1.ApicurioRegistry; | ||
| import io.apicurio.registry.operator.api.v3.ApicurioRegistry3; |
There was a problem hiding this comment.
ApicurioRegistry3 kind has version v1, even though it is intended for Registry v3. Either use io.apicurio.registry.operator.api.v1 or what I did io.apicurio.registry.operator.api.v3.v1.
| @Plural("ApicurioRegistries3") | ||
| public class ApicurioRegistry3 extends CustomResource<ApicurioRegistry3Spec, ApicurioRegistry3Status> | ||
| implements Namespaced { | ||
| } No newline at end of file |
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| @JsonPropertyOrder({ "conditions", "info" }) | ||
| @JsonDeserialize(using = JsonDeserializer.None.class) | ||
| public class ApicurioRegistry3Status implements KubernetesResource { |
There was a problem hiding this comment.
Use io.javaoperatorsdk.operator.api.ObservedGenerationAwareStatus. The framework then updates the observed generation automatically.
There was a problem hiding this comment.
this adds the dependency on the JOSDK to the model package ... but ok ..
| @JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_NULL) | ||
| @JsonPropertyOrder({ "env" }) | ||
| @JsonDeserialize(using = JsonDeserializer.None.class) | ||
| public class Configuration implements KubernetesResource { |
There was a problem hiding this comment.
Why not use builder:
@Buildable(editableEnabled = false, builderPackage = "io.fabric8.kubernetes.api.builder")
| @JsonInclude(com.fasterxml.jackson.annotation.JsonInclude.Include.NON_NULL) | ||
| @JsonPropertyOrder({ "env" }) | ||
| @JsonDeserialize(using = JsonDeserializer.None.class) | ||
| public class Configuration implements KubernetesResource { |
There was a problem hiding this comment.
Why not use Lombok? (Also elsewhere). All fabric8 models use Lombok and it's far more readable.
| @Required() | ||
| @JsonPropertyDescription("lastTransitionTime is the last time the condition transitioned from one status to another. This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable.") | ||
| @JsonSetter(nulls = com.fasterxml.jackson.annotation.Nulls.SKIP) | ||
| private ZonedDateTime lastTransitionTime; |
There was a problem hiding this comment.
no super strong opinions here, ok
| return lastTransitionTime; | ||
| } | ||
|
|
||
| @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss[XXX][VV]") |
There was a problem hiding this comment.
Why there are two different formats for getter and setter?
There was a problem hiding this comment.
A long long story here, basically:
fabric8io/kubernetes-client#5384 (comment)
happy to share the full story when needed XD
| this.reason = reason; | ||
| } | ||
|
|
||
| public enum Status { |
There was a problem hiding this comment.
Move these to a separate class please for better readability.
|
|
||
| // Assert | ||
| assertEquals("test", registry.getMetadata().getName()); | ||
| assertEquals("test-namespace", registry.getMetadata().getNamespace()); |
There was a problem hiding this comment.
I'd like to see a comparison with a manually (with code) created instance (with status, since spec is empty). Builders help here.
| assertEquals("test", registry.getMetadata().getName()); | ||
| assertEquals("test-namespace", registry.getMetadata().getNamespace()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is explicit ApicurioRegistry3List unnecessary? I'd like to see a test that reads a list of CRDs to see an example.
There was a problem hiding this comment.
added an example on how to deserialize a List of Registries
|
Hey guys, wtf is this? What I'm sure I've asked was: Andrea: code reviews Can anyone help me to understand how we ended up with the exact opposite? |
This was opened last week. We can close it, but I didn't want to close someone else's PR. |
|
np with not closing it, but why the Code review has been done today? |
|
Done everything, but the changes related to |
|
added lombok, @jsenko ready for next round. |
jsenko
force-pushed
the
switch-to-code-first
branch
from
81c2989 to
99c5193
Compare
3 participants
No description provided.