Skip to content

Provide conversion webhook for CRDs#49

Merged
sgraband merged 2 commits intomainfrom
conversion-webhook
Mar 4, 2024
Merged

Provide conversion webhook for CRDs#49
sgraband merged 2 commits intomainfrom
conversion-webhook

Conversation

@sgraband
Copy link
Contributor

Webhook is called, whenever a CR is requested in a specific version. This is enabled from these versions:

  • AppDefintion.v1beta8
  • Session.v1beta6
  • Workspace.v1beta3 Older versions are deprecated and no longer in the definition.

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.

Contributed on behalf of STMicroelectronics
Co-authored-by: Johannes Faltermeier jfaltermeier@eclipsesource.com

Main Repo PR:

Webhook is called, whenever a CR is requested in a specific version.
This is enabled from these versions:
- `AppDefintion.v1beta8`
- `Session.v1beta6`
- `Workspace.v1beta3`
Older versions are deprecated and no longer in the definition.

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.

Contributed on behalf of STMicroelectronics
Co-authored-by: Johannes Faltermeier <jfaltermeier@eclipsesource.com>
Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a first look without testing.
I've only noticed one small issue

dnsNames:
- "conversion-webhook-service.{{ .Release.Namespace }}.svc"
issuerRef:
name: theia-cloud-selfsigned-issuer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible to change the name of the selfsigned issuer in the base chart with issuerstaging.name. So this might not work in all cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes good catch. I will make this configurable via the values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also removed two unsued values from the main chart in the commit below.

Also remove unused values from main chart

Contributed on behalf of STMicroelectronics
Copy link
Contributor

@jfaltermeier jfaltermeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lgtm!

@sgraband sgraband merged commit bea96bc into main Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants