-
Notifications
You must be signed in to change notification settings - Fork 37
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
Provide conversion webhook for CRDs #283
Conversation
Webhook is called, whenever a CR is requested in a specific version. This allows for a better updating path, as old CRs are still useable. [conversion] Provide conversion java project: - A webhook that offers endpoints for the three CRD types - Is buildable as a runnable jar - Contains Mappers from all supported versions to a hub [common] Add Hub and previous version for all CRDs - The hub is a superset of all values of a CRD across all versions - Keep a copy of the old supported versions to transform back - This is useful, as this way the the old operator still works - Supported from these versions: - `AppDefintion.v1beta8` - `Session.v1beta6` - `Workspace.v1beta3` - To showcase the functionality update CRDs to new version: - Move status like fields to status: - `Session.v1beta7`: Move `url`, `lastActivity` and `error` fields from the spec to the status. - `Workspace.v1beta4`: Move the `error` field from the spec to the status. Also add the `error` field to `Workspace.v1beta3` as it was missing - Remove `timeout.strategy` from AppDefinition - `AppDefinition.v1beta9`: Removed `timeout.strategy` and `timeout.limit` is now just `timeout`. This was done, as there is only one Strategy left. [operator] Adjust operator so it works with the above changes. [service] Adjust service so it works with the above changes. [documentation] Add build command for `conversion-webhook` [.github] Add ci for `conversion-webhook` - Also fix publishing of new `next-version` (reusable typo) Contributed on behalf of STMicroelectronics Co-authored-by: Johannes Faltermeier <[email protected]>
* add eclipse metadata to be consistent with other java components * open all projects in Theia IDE with Java once to update settings
I've pushed some missing eclipse metadata files and opened all java projects with Eclipse (2023-03), VSCode, and the TheiaIDE and committed the resulting updates to the metadata files. |
I noticed that there are some left over theia.cloud/v1beta8 AppDefinition references in: |
* add eclipse metadata to be consistent with other java components * open all projects in Theia IDE with Java once to update settings
Could you please check if you get changes on the settings in different IDEs? If yes we should check if we can fix this (probably in a follow up ticket), because the settings should not get added to |
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, I had a first look at the code and it looks very good already.
I will start some testing now as well.
...ain/java/org/eclipse/theia/cloud/common/k8s/resource/appdefinition/hub/AppDefinitionHub.java
Outdated
Show resolved
Hide resolved
....service/src/test/java/org/eclipse/theia/cloud/service/workspace/WorkspaceResourceTests.java
Outdated
Show resolved
Hide resolved
...ia.cloud.conversion/src/main/java/org/eclipse/theia/cloud/conversion/ConversionEndpoint.java
Outdated
Show resolved
Hide resolved
Looks good on my side. Tested also with VSCode, TheiaIDE and Eclipse |
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.
I did a first test and found one issue (see inline comment).
Overall everything looks pretty good. I had no issues with certificates and the conversion was triggered smoothly.
I will do more testing on Friday
...common/src/main/java/org/eclipse/theia/cloud/common/k8s/resource/session/hub/SessionHub.java
Outdated
Show resolved
Hide resolved
Update `v1beta8` references to `v1beta9`. Fix `sessionSecret` in `SessionHub`. Fix tests. Use `org.jboss.logging.Logger` in conversion project. Also log the endpoints that were triggered.
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.
I've found two more mixups in the Hub.
After those are fixed, I think we can approve and merge this.
This was the only issue I found during the second test.
Also I pushed one commit updating the AppDefinitions in the tf files I used for testing
...ain/java/org/eclipse/theia/cloud/common/k8s/resource/appdefinition/hub/AppDefinitionHub.java
Outdated
Show resolved
Hide resolved
...ain/java/org/eclipse/theia/cloud/common/k8s/resource/appdefinition/hub/AppDefinitionHub.java
Outdated
Show resolved
Hide resolved
...ain/java/org/eclipse/theia/cloud/common/k8s/resource/appdefinition/hub/AppDefinitionHub.java
Outdated
Show resolved
Hide resolved
Fix minor issues in constructors.
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!
Webhook is called, whenever a CR is requested in a specific version. This allows for a better updating path, as old CRs are still useable.
[conversion] Provide conversion java project:
[common] Add Hub and previous version for all CRDs
The hub is a superset of all values of a CRD across all versions
Keep a copy of the old supported versions to transform back
This is useful, as this way the the old operator still works
Supported from these versions: -
AppDefintion.v1beta8
-Session.v1beta6
-Workspace.v1beta3
To showcase the functionality update CRDs to new version: - Move status like fields to status: -
Session.v1beta7
: Moveurl
,lastActivity
anderror
fields from the spec to the status. -Workspace.v1beta4
: Move theerror
field from the spec to the status. Also add theerror
field toWorkspace.v1beta3
as it was missingtimeout.strategy
from AppDefinitionAppDefinition.v1beta9
: Removedtimeout.strategy
andtimeout.limit
is now just
timeout
. This was done, as there is only one Strategy left.[operator] Adjust operator so it works with the above changes.
[service] Adjust service so it works with the above changes.
[documentation] Add build command for
conversion-webhook
[.github] Add ci for
conversion-webhook
next-version
(reusable typo)Contributed on behalf of STMicroelectronics
Co-authored-by: Johannes Faltermeier [email protected]
Helm-repo PR: eclipse-theia/theia-cloud-helm#49
Fixes #203, #212 and #161.