-
Notifications
You must be signed in to change notification settings - Fork 33
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
Update kubernetes-client and quarkus #287
Conversation
The changes itself look good to me. https://mvnrepository.com/artifact/io.javaoperatorsdk/kubernetes-webhooks-framework-core I noticed something different however.
I don't know if the cause for this is this update or if this somehow slipped through with the conversion webhook change. |
The issue seems like an oversight from the previous PR. I added a commit that should resolve the issue. Regarding the |
Keeping the the webhooks version is fine for. I got two more exceptions when launching a session. Service:
Conversion Hook:
|
In order to avoid such problems in the future i added a safe guard always return a empty status, when the status is null. This way we do not need to check if the status is null everytime we use it. WDYT? (I also reverted the previous change as it should no longer be necessary) |
...n/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/appdefinition/AppDefinition.java
Outdated
Show resolved
Hide resolved
Update kubernetes-client and quarkus platform. Also fix a deprecated warning.
To avoid endless restarting loops when k8s-client watcher keeps reconnecting.
Add getNonNullStatus() method, that returns an empty Status if none available. This way you can check for values with out having to worry about NPEs. Properly set the status via the provided method. With simply edit the status is not applied. Provide toString() methods for the status objects.
582ee44
to
ddc7100
Compare
@jfaltermeier do you have an idea on why the test might fail? I just took a look and it seems like that specific failing test seems to not have the Also, mocking the result for |
Adding |
I could not see errors in the service or the operator anymore. In the conversion webhook there is still a NPE regarding Sessions. AppDefinitions and Workspaces seemed to work
|
I think this is the issue:
|
Thanks! I will take look 👍 Another question: Did the current state work for you? I totally forgot to open and link that PR, but i also needed these changes: eclipse-theia/theia-cloud-helm#51. Without them i still recieved some errors, while creating sessions. |
I pasted the error I saw, but I did no detailed testing after that. |
This fixes the issue on my side. Also i updated the description, that eclipse-theia/theia-cloud-helm#51 should be used. Could you recheck? |
The This was with the vscode exception:
|
...rator/src/main/java/org/eclipse/theia/cloud/operator/monitor/MonitorActivityTrackerImpl.java
Outdated
Show resolved
Hide resolved
This way we get the updated resource and can reassign the object.
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, LGTM! 🥳
Update kubernetes-client to 6.10.0.
Update quarkus platform to 3.8.1.
This provides support for kubernetes 1.29.0.
Additionaly fix some deprecated warnings.
Helm Repo PR: eclipse-theia/theia-cloud-helm#51