-
Notifications
You must be signed in to change notification settings - Fork 26
Update JOSDK to v5.1.2 #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
im-konge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, few comments.
operator/src/test/java/io/strimzi/kafka/access/KafkaAccessReconcilerTest.java
Show resolved
Hide resolved
operator/src/test/java/io/strimzi/kafka/access/SecretDependentResourceTest.java
Outdated
Show resolved
Hide resolved
operator/src/test/java/io/strimzi/kafka/access/SecretDependentResourceTest.java
Outdated
Show resolved
Hide resolved
ec144f7 to
a7b7659
Compare
katheris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @OwenCorrigan76. I think we need some additional changes for the switch to SSA and I don't think the Kafka Access Secret event source is being used correctly. Also from reviewing the migration docs I think we need to change the way we use the SDK generation to use the maven plugin.
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessStatus.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessStatus.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Show resolved
Hide resolved
operator/src/test/java/io/strimzi/kafka/access/KafkaAccessReconcilerTest.java
Outdated
Show resolved
Hide resolved
ceca15e to
dedd7b5
Compare
|
@OwenCorrigan76 about the unused declared dependency -> you removed the |
That's exactly the reason. Thanks @im-konge |
03a26d0 to
e063acb
Compare
im-konge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes, few more comments.
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Show resolved
Hide resolved
operator/src/test/java/io/strimzi/kafka/access/KafkaAccessReconcilerTest.java
Outdated
Show resolved
Hide resolved
katheris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @OwenCorrigan76 I think we're pretty close now, but a couple more changes needed.
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessStatus.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/strimzi/kafka/access/model/KafkaAccessStatus.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/io/strimzi/kafka/access/KafkaAccessReconciler.java
Show resolved
Hide resolved
operator/src/test/java/io/strimzi/kafka/access/KafkaAccessReconcilerTest.java
Show resolved
Hide resolved
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
Signed-off-by: OwenCorrigan76 <[email protected]>
07fb651 to
1c73e14
Compare
Signed-off-by: OwenCorrigan76 <[email protected]>
|
@katheris @im-konge When I have the SSA flag in the operator initialization, I get the following error when deploying the I couldn't figure out where the error was even coming from until i removed the flag and went back to: Now it builds and works fine. I can't see where the issue lies. Would you be able to have a look if you have some time? Thanks |
|
@OwenCorrigan76 the log you shared is a bit cut. Could you share the full log possibly? |
|
@im-konge I updated the full log there now. Thanks |
|
It says something about "unprocessable entity" - so possibly you have some field wrong there? How does the KafkaAccess look like? Isn't it failing because of the observedGeneration? |
|
Also you can for example check how does the patch actually look like, what you are updating there etc. |
|
The KafkaAccess looks like the following: These are the two points where we are using it [1] [2]. In v4.x we used In v5.x |
|
Also, I treid removing |
|
What SSA flag you mean? |
|
If you have some SSA flag set there, which configures the operator to use SSA, we discussed that you should disable that and handle the SSA in different PR. Maybe I'm blind but I don't see any flag like that in the code. Is it happening in the unit tests? Is it happening when you deploy the operator on Kube? |
|
Sorry @im-konge. I removed it in the last commit to see would it builld and pass the tests (which it did). This is the flag that is breaking everything. And the flag is used to explicity NOT use SSA as discussed here. kafka-access-operator/operator/src/main/java/io/strimzi/kafka/access/KafkaAccessOperator.java Lines 29 to 30 in 03feca3
|
So that means it's using the SSA right? It should be IMO consistent in the main class and in the test class as well, otherwise you are testing and running two different things and two different behaviors. |
Signed-off-by: OwenCorrigan76 <[email protected]>
|
@im-konge Yes I agree. I have reverted it to have the the flag with |
No worries, I think that the building the status from scratch will help. Let's see if it will work (I checked the branch and there is no other way - just the patch 😄 so we need to fix it). |
|
Hey @OwenCorrigan76 I took a look at the error you're getting and I think it is related to the change to use the So I tested with your branch and removing the |
Signed-off-by: OwenCorrigan76 <[email protected]>
|
@katheris Thanks so much for looking at this and discovering the issue. I have tested it successfully and pushed the changes. |
im-konge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM, thanks for the changes.
Just one more question about the status issues during patch phase - I saw that you removed the init status (as it caused the exception you mentioned earlier) -> does it work fine when you update the KafkaAccess resource with different stuff, so the status section of the KafkaAccess resource will be updated once more (so from Ready to "not ready" or something like that and back to the Ready state)
|
@im-konge The status is updating fine. For example when deploying with the incorrect namespace: and then with the correct namespace: Is that the type of thing you mean? |
Yes, thanks |
|
@katheris are we waiting for something else or we can merge this? |
|
@im-konge I reached out to the Java Operator SDK community to see if anyone has time to review this PR. I would say let's give them until Wednesday and if no one responds to my message I'll merge it |
xstefank
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From JOSDK side, LGTM
|
Thanks @xstefank for giving it a look over |
This PR migrates the Java Operator SDK from v4.4.2 to v5.1.2. The updated vesrion will allow Access Operator to watch Kafka clusters in a remote Kubernetes cluster, as requested in issue #44.
This PR directly addresses the follwoing issue: #89