feat: migrate AppDefinitions to v1beta11 sidecars#73
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps AppDefinition CRD to theia.cloud/v1beta11 and adds optional per-app Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant K8sAPI as Kubernetes API
participant Controller
participant Pod as IDE+Sidecar Pod
User->>Helm: helm install/upgrade (values include apps + sidecars)
Helm->>K8sAPI: apply AppDefinition (theia.cloud/v1beta11)
K8sAPI->>Controller: controller sees AppDefinition
Controller->>K8sAPI: create/update Deployment/PodSpec (main container + sidecar(s))
K8sAPI->>Pod: schedule Pod (IDE container + sidecar containers)
Pod->>Pod: IDE ↔ language-server sidecar communication
Pod->>K8sAPI: readiness/status updates
K8sAPI->>User: deployment ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Migrates the test3 deployment’s AppDefinitions to the Theia Cloud v1beta11 API and introduces sidecar language server rendering so test3 can use the new external language server architecture (per EduIDE-Cloud PR #70).
Changes:
- Update AppDefinition
apiVersionand Helmlookuptotheia.cloud/v1beta11. - Add Helm templating support for
spec.sidecars[]in the AppDefinition chart. - Update
test3values to define Java/Rust “no-ls” apps using sidecar language servers and adjust landing page copy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| deployments/test3.theia-test.artemis.cit.tum.de/values.yaml | Switches test3 app config to sidecar-based language servers and updates landing page app entries/text. |
| charts/theia-appdefinitions/templates/appdefinition.yaml | Migrates AppDefinition manifests to v1beta11 and adds sidecars rendering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml (2)
144-163: Avoid mutablelatesttags for test3 migration images.Lines 146, 150, 156, and 160 use
latest, which makes deployments non-reproducible and can break IDE/sidecar compatibility unexpectedly.Suggested pinning pattern
- imageTag: latest + imageTag: pr-70 @@ - image: ghcr.io/ls1intum/theia/langserver-java:latest + image: ghcr.io/ls1intum/theia/langserver-java:pr-70 @@ - imageTag: latest + imageTag: pr-70 @@ - image: ghcr.io/ls1intum/theia/langserver-rust:latest + image: ghcr.io/ls1intum/theia/langserver-rust:pr-70🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/test3.theia-test.artemis.cit.tum.de/values.yaml` around lines 144 - 163, The imageTag fields for the deployment entries java-17-no-ls and rust-no-ls (and their sidecar images ghcr.io/ls1intum/theia/langserver-java and ghcr.io/ls1intum/theia/langserver-rust) are set to the mutable "latest"; replace these with specific, immutable version tags (e.g., semver or commit SHA) for both the main images (image/imageTag) and the sidecar images (sidecars[].image) so the deployment becomes reproducible and avoids unexpected compatibility breaks—update the imageTag values and ensure the sidecar image references use the pinned tags as well.
144-163: Add the new no-ls/langserver images to preloading to reduce cold starts.These apps introduce new images, but the preloading list still points to older sets, so first-user startup may regress.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/test3.theia-test.artemis.cit.tum.de/values.yaml` around lines 144 - 163, The new app entries 'java-17-no-ls' and 'rust-no-ls' add container images and sidecar images that are not included in the preloading list, causing potential cold-start regressions; update the preloading section to include the four new image references (ghcr.io/ls1intum/theia/java-17-no-ls:latest, ghcr.io/ls1intum/theia/langserver-java:latest, ghcr.io/ls1intum/theia/rust-no-ls:latest, ghcr.io/ls1intum/theia/langserver-rust:latest) so both the main images and their langserver sidecars are pre-pulled/primed at startup. Ensure the exact image tags match the imageTag fields used for 'java-17-no-ls' and 'rust-no-ls' and add them to the same preloading list format used elsewhere in values.yaml.charts/theia-appdefinitions/templates/appdefinition.yaml (1)
2-17: Fail fast if v1beta11 CRDs are not installed yet.Line 2 and Line 17 hard-require
theia.cloud/v1beta11. Add a capability guard so installs fail early with a clear message instead of failing later during apply.Suggested guard
+{{- if not (.Capabilities.APIVersions.Has "theia.cloud/v1beta11/AppDefinition") }} +{{- fail "AppDefinition v1beta11 is required. Install the PR `#70` CRDs/operator before deploying this chart." }} +{{- end }} {{- range .Values.apps }} {{- $existingApp := lookup "theia.cloud/v1beta11" "AppDefinition" $.Release.Namespace .name }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/theia-appdefinitions/templates/appdefinition.yaml` around lines 2 - 17, Add a capability guard that checks for the AppDefinition CRD before referencing the theia.cloud/v1beta11 API: use lookup to query the CRD (e.g., lookup "apiextensions.k8s.io/v1" "CustomResourceDefinition" "" "appdefinitions.theia.cloud") at the top of the template and call fail with a clear message if it is nil, so the existing lookup "theia.cloud/v1beta11" "AppDefinition" and the apiVersion: theia.cloud/v1beta11 usage do not proceed when the CRD is not installed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/theia-appdefinitions/templates/appdefinition.yaml`:
- Around line 57-83: The template currently forces mountWorkspace to true
because the Helm default filter treats false as empty; update the sidecar
rendering (inside the {{- range .sidecars }} loop) to detect whether the sidecar
object actually provides a mountWorkspace field using hasKey (e.g., if hasKey .
"mountWorkspace") and render mountWorkspace with the provided value when
present, otherwise render the default true; reference the mountWorkspace field
and the sidecar iteration (the .sidecars range and the individual sidecar object
represented by .) when making the change.
---
Nitpick comments:
In `@charts/theia-appdefinitions/templates/appdefinition.yaml`:
- Around line 2-17: Add a capability guard that checks for the AppDefinition CRD
before referencing the theia.cloud/v1beta11 API: use lookup to query the CRD
(e.g., lookup "apiextensions.k8s.io/v1" "CustomResourceDefinition" ""
"appdefinitions.theia.cloud") at the top of the template and call fail with a
clear message if it is nil, so the existing lookup "theia.cloud/v1beta11"
"AppDefinition" and the apiVersion: theia.cloud/v1beta11 usage do not proceed
when the CRD is not installed.
In `@deployments/test3.theia-test.artemis.cit.tum.de/values.yaml`:
- Around line 144-163: The imageTag fields for the deployment entries
java-17-no-ls and rust-no-ls (and their sidecar images
ghcr.io/ls1intum/theia/langserver-java and
ghcr.io/ls1intum/theia/langserver-rust) are set to the mutable "latest"; replace
these with specific, immutable version tags (e.g., semver or commit SHA) for
both the main images (image/imageTag) and the sidecar images (sidecars[].image)
so the deployment becomes reproducible and avoids unexpected compatibility
breaks—update the imageTag values and ensure the sidecar image references use
the pinned tags as well.
- Around line 144-163: The new app entries 'java-17-no-ls' and 'rust-no-ls' add
container images and sidecar images that are not included in the preloading
list, causing potential cold-start regressions; update the preloading section to
include the four new image references
(ghcr.io/ls1intum/theia/java-17-no-ls:latest,
ghcr.io/ls1intum/theia/langserver-java:latest,
ghcr.io/ls1intum/theia/rust-no-ls:latest,
ghcr.io/ls1intum/theia/langserver-rust:latest) so both the main images and their
langserver sidecars are pre-pulled/primed at startup. Ensure the exact image
tags match the imageTag fields used for 'java-17-no-ls' and 'rust-no-ls' and add
them to the same preloading list format used elsewhere in values.yaml.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68b092d0-3683-4ae4-aea6-ee052105affd
📒 Files selected for processing (2)
charts/theia-appdefinitions/templates/appdefinition.yamldeployments/test3.theia-test.artemis.cit.tum.de/values.yaml
Aligns with EduIDE-Helm PR #11 which adds v1beta11 support. With helm_chart_tag=pr-11, CI will now resolve theia-cloud-crds:1.2.0-next.1.pr-11 from GHCR.
Points theia-cloud-crds at the eduide-fork conversion-webhook image (conversion-webhook:pr-70) which includes the v1beta11 AppDefinition mapper required for the sidecar redesign.
There was a problem hiding this comment.
Pull request overview
Migrates the test3 environment’s AppDefinitions to the theia.cloud/v1beta11 API and introduces Helm rendering support for spec.sidecars[], aligning the deployment with the external language-server sidecar architecture referenced from EduIDE-Cloud PR #70.
Changes:
- Update
theia-appdefinitionschart template to renderAppDefinitionresources astheia.cloud/v1beta11and addspec.sidecars[]support. - Migrate
test3environment app configs from legacyoptions.langserver-imageto structuredsidecarsconfiguration and adjust landing page text/options. - Bump the upstream CRDs chart version in the deployment workflow and add a conversion webhook image override for
test3.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml |
Migrates test3 AppDefinitions to sidecars and updates landing page apps/text accordingly. |
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Adds conversion webhook image override for v1beta11 migration in test3. |
charts/theia-appdefinitions/templates/appdefinition.yaml |
Switches AppDefinition rendering/lookup to v1beta11 and adds sidecar rendering. |
.github/workflows/deploy-theia.yml |
Bumps CRDs chart version to match the new API level. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
theia-no-ls:pr-46 and langserver-java:pr-46 no longer exist in the registry. The no-ls AppDefinitions reference images from ls1intum/theia directly, so these preloading entries served no purpose.
There was a problem hiding this comment.
Pull request overview
Migrates the Theia AppDefinition rendering and the test3 environment configuration to the v1beta11 sidecar-based language server architecture (aligned with EduIDE-Cloud PR #70), and updates deployment inputs accordingly.
Changes:
- Update
theia-appdefinitionschart to rendertheia.cloud/v1beta11AppDefinitions and supportspec.sidecars[]. - Migrate
test3deployment values from legacyoptions.langserver-imageto structuredsidecarsand update landing page/default apps. - Pin CRD conversion webhook image in environment CRD values, and update the deploy workflow’s CRD chart version + preloaded images.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRD chart |
| deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRD chart |
| deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRD chart |
| deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRD chart |
| deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRD chart |
| deployments/test3.theia-test.artemis.cit.tum.de/values.yaml | Switches test3 to sidecar-based app definitions + updates landing page defaults |
| charts/theia-cloud-combined/values.yaml | Adjusts garbage collector image repository |
| charts/theia-appdefinitions/templates/appdefinition.yaml | Bumps AppDefinition apiVersion to v1beta11 and renders spec.sidecars[] |
| .github/workflows/deploy-theia.yml | Bumps CRD chart version and preloads new sidecar/No-LS images |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
…gs (#75) - Change theia_cloud_tag and ide_images_tag defaults from 'latest' to '' - Update input descriptions to clarify empty means no override - deploy-pr.yml: forward empty tags as-is (remove || 'latest' fallback) - deploy-theia.yml: apply --set image overrides only when tag is non-empty, otherwise leave values.yaml settings untouched; add set -euo pipefail
- Change theia_cloud_tag and ide_images_tag defaults from 'latest' to '' - Update input descriptions to clarify empty means no override - deploy-pr.yml: forward empty tags as-is (remove || 'latest' fallback) - deploy-theia.yml: apply --set image overrides only when tag is non-empty, otherwise leave values.yaml settings untouched; add set -euo pipefail
When dispatching manually, selecting 'all' (the new default) triggers all three deploy-test* jobs in a single run. Each job is still gated by its individual environment approval, so operators can approve only the environments they want without having to re-trigger the workflow for each one.
…nly' into feat/test3-pr70-sidecar-clean
There was a problem hiding this comment.
Pull request overview
Migrates the Theia AppDefinition rendering and the test3 deployment to the v1beta11 sidecar-based language-server architecture, and adjusts deployment workflows/CRD configuration to support the v1beta11 API/conversion.
Changes:
- Update
theia-appdefinitionschart to render AppDefinitions astheia.cloud/v1beta11and addspec.sidecars[]rendering support. - Migrate
test3environment values from legacyoptions.langserver-imageto structuredsidecarsapp definitions and adjust landing page defaults. - Update GitHub Actions deployment workflows to make image tag overrides optional (empty means “no override”) and bump the CRDs chart version; pin conversion webhook image in CRD helm values.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
charts/theia-appdefinitions/templates/appdefinition.yaml |
Switch lookup/apiVersion to v1beta11 and render spec.sidecars[]. |
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml |
Migrate test3 app definitions to sidecar config; adjust preloading and landing page defaults/text. |
deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml |
Add pinned conversion webhook image to CRDs values. |
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml |
Add pinned conversion webhook image to CRDs values. |
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Add pinned conversion webhook image to CRDs values. |
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Add pinned conversion webhook image to CRDs values. |
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Add pinned conversion webhook image to CRDs values (test3). |
charts/theia-cloud-combined/values.yaml |
Adjust garbage-collector image repository value. |
.github/workflows/deploy-theia.yml |
Make image overrides conditional on non-empty inputs; bump CRDs chart version; refactor helm command construction. |
.github/workflows/deploy-pr.yml |
Pass tag inputs through without defaulting to latest (enables “no override”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Migrates Theia Cloud AppDefinitions and the test3 environment configuration to the v1beta11 sidecar-based language-server architecture (aligned with EduIDE-Cloud PR #70), while adjusting deployment workflows to support “no override” image-tag inputs.
Changes:
- Update theia-appdefinitions chart to render
theia.cloud/v1beta11AppDefinitions and addspec.sidecars[]rendering. - Migrate
test3environment values to sidecar-based language servers (new app definitions + updated preloading/landing page defaults). - Adjust GitHub Actions deployment workflows to only apply image overrides when input tags are explicitly provided, and bump CRDs chart version.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs deployment (prod). |
| deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs deployment (staging). |
| deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs deployment (test1). |
| deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs deployment (test2). |
| deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs deployment (test3). |
| deployments/test3.theia-test.artemis.cit.tum.de/values.yaml | Migrates test3 to sidecar-based app definitions + updates images/landing page config. |
| charts/theia-appdefinitions/templates/appdefinition.yaml | Switches AppDefinition apiVersion/lookup to v1beta11 and renders spec.sidecars. |
| .github/workflows/deploy-theia.yml | Makes tag overrides conditional on non-empty inputs; bumps CRDs chart version. |
| .github/workflows/deploy-pr.yml | Changes manual dispatch behavior to trigger all env jobs; updates tag-input defaults. |
Comments suppressed due to low confidence (1)
.github/workflows/deploy-theia.yml:227
- The workflow comment says the TLS cert/key secrets are already base64-encoded, but this step base64-encodes them again before passing them into
theia-certificates(which expects base64 data). This is confusing and can lead to misconfigured secrets if someone follows the comment literally; please clarify whether the GitHub secrets should be raw PEM or base64 and update the comment accordingly (docs/tum-certificates.md suggests raw PEM).
# Prepare SSL certificates for ingress
# Note: Secrets are already base64 encoded, but we need to write them to files
echo "${{ secrets.THEIA_WILDCARD_CERTIFICATE_CERT }}" | base64 -w 0 > wildcard.crt
echo "${{ secrets.THEIA_WILDCARD_CERTIFICATE_KEY }}" | base64 -w 0 > wildcard.key
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Migrates Theia AppDefinitions to the v1beta11 sidecar-based language server architecture and updates the deployment tooling/config to support that rollout (notably for the test3 environment, plus workflow/CRD/RBAC adjustments).
Changes:
- Update Helm AppDefinition rendering to
theia.cloud/v1beta11and addspec.sidecars[]support. - Migrate
test3deployment values from legacy langserver options to structured sidecars, including new no-LS IDE images. - Adjust deployment workflows/CRD values and add RBAC needed for sidecar-related pod management and conversion webhook configuration.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
charts/theia-appdefinitions/templates/appdefinition.yaml |
Switches AppDefinition apiVersion/lookup to v1beta11 and renders spec.sidecars. |
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml |
Migrates test3 to sidecar-based appdefs and updates images/landing page config. |
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Configures CRD conversion webhook image for test3. |
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Adds CRD conversion webhook configuration for test1. |
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Adds CRD conversion webhook configuration for test2. |
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml |
Adds CRD conversion webhook configuration for staging. |
deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml |
Adds CRD conversion webhook configuration for production. |
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml |
Introduces Role/RoleBinding to allow the operator to delete pods for sidecar restart behavior. |
.github/workflows/deploy-theia.yml |
Makes image-tag overrides optional (empty = no override), updates CRD chart version, and refactors Helm command construction. |
.github/workflows/deploy-pr.yml |
Updates manual dispatch behavior/inputs to pass optional tag overrides into the reusable deploy workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml
Show resolved
Hide resolved
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
Show resolved
Hide resolved
…oad override guard, docs fix, and RBAC least-privilege adjustment
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/deploy-pr.yml (1)
37-90: Make the caller’sGITHUB_TOKENscope explicit.These three reusable-workflow jobs still rely on an implicit token scope. Even if
.github/workflows/deploy-theia.ymlalready narrows it, declaring the minimum permissions here avoids accidental write-by-default access and matches the CodeQL warning on this file.Minimal starting point
name: Deploy PR to Environment +permissions: + contents: read + packages: read + on:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-pr.yml around lines 37 - 90, Add an explicit permissions stanza to each reusable-workflow job (deploy-test1, deploy-test2, deploy-test3) to avoid implicit write access by GITHUB_TOKEN; insert a permissions block under each job with the minimal scopes required (for example: permissions: contents: read, packages: read, actions: read) so the caller declares the GITHUB_TOKEN scope explicitly instead of inheriting default write permissions from the workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml`:
- Around line 8-11: The Role's verbs currently include "deletecollection" on the
"pods" resource which grants bulk-delete capability; remove "deletecollection"
from the verbs array in the Role definition (the rules block that lists
apiGroups, resources: ["pods"], verbs: [...]) so that only individual pod
deletions (e.g., "delete") are permitted unless you can justify bulk-deletes;
update the Role manifest (rbac-operator-sidecar-pod-restart.yaml) to drop
"deletecollection" and keep only least-privilege verbs needed for the restart
flow.
---
Nitpick comments:
In @.github/workflows/deploy-pr.yml:
- Around line 37-90: Add an explicit permissions stanza to each
reusable-workflow job (deploy-test1, deploy-test2, deploy-test3) to avoid
implicit write access by GITHUB_TOKEN; insert a permissions block under each job
with the minimal scopes required (for example: permissions: contents: read,
packages: read, actions: read) so the caller declares the GITHUB_TOKEN scope
explicitly instead of inheriting default write permissions from the workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a014b31-1f71-4d64-9f63-61fb16d02fc2
📒 Files selected for processing (9)
.github/workflows/deploy-pr.yml.github/workflows/deploy-theia.ymlcharts/theia-appdefinitions/templates/appdefinition.yamlcharts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yamldeployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.ymldeployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.ymldeployments/test3.theia-test.artemis.cit.tum.de/values.yamldeployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.ymldeployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml
✅ Files skipped from review due to trivial changes (4)
- deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
- deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml
- deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml
- deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/deploy-theia.yml
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml
Show resolved
Hide resolved
|
Pushed commit 68dd3a6 addressing the actionable unresolved review items:\n- add v1beta10 fallback for AppDefinition lookup during migration safety\n- align test3 control-plane/runtime tags to pr-70 (operator/service/landing/no-ls/langserver/defaultImageTag)\n- gate no-ls preload index overrides behind include_no_ls_preloads=true (enabled only for test3 in deploy-pr)\n- update docs/adding-environments.md to reflect deploy-pr manual dispatch behavior\n- reduce RBAC scope by removing pod deletecollection\n\nFor remaining conversion-webhook pin comments across envs: the pr-70 pin is intentional during this migration validation window; we can unpin/parameterize in a dedicated follow-up after rollout verification. |
…10 fallback and RBAC over-permissioning
|
Applied your requested direction changes and pushed commit f393360:\n- reverted v1beta10 fallback (lookup remains strict v1beta11 only)\n- restored deletecollection in sidecar restart RBAC\n- added execution_mode dropdown in deploy-pr (self-hosted-buildkit/github-runners)\n- wired execution_mode through deploy-theia to route runs-on (ARC default, GH fallback)\n- kept global IDE preload override behavior (including indices 7..10) when ide_images_tag is set\n\nThread resolution status:\n- Resolved addressed threads: docs dispatch behavior, service tag mismatch, PR scope/description clarification\n- Remaining open (not resolved, since not addressed by current requested direction):\n 1) v1beta10 fallback recommendations (2 threads)\n 2) conversion-webhook pr-70 pin concerns across env CRD values (4 threads)\n 3) two outdated threads that remain open but are stale (preloading env-awareness, deletecollection least-privilege)\n\nIf you want, I can also close the two outdated stale threads now. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the test3 environment toward the v1beta11 AppDefinition + sidecar architecture to validate EduIDE-Cloud PR #70 images, and updates the deployment workflows/docs to match the new deployment behavior.
Changes:
- Update the
theia-appdefinitionsHelm template to rendertheia.cloud/v1beta11AppDefinitions and addspec.sidecarsrendering. - Migrate
test3environment values to sidecar-based app definitions and pin migration-validation images/tags. - Adjust GitHub Actions deployment workflows to remove the manual environment selector, make image tag overrides optional (empty = no override), and add runner selection via
execution_mode.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/adding-environments.md | Updates environment-adding instructions to match the current deploy-pr workflow behavior. |
| deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs install values. |
| deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs install values. |
| deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs install values. |
| deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Adds conversion webhook image override for CRDs install values. |
| deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml | Improves issuer comment and pins conversion webhook image for test3 CRDs install values. |
| deployments/test3.theia-test.artemis.cit.tum.de/values.yaml | Pins test3 control-plane images and migrates app definitions to sidecar format. |
| charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml | Introduces a Role/RoleBinding intended to allow operator-initiated pod restart operations. |
| charts/theia-appdefinitions/templates/appdefinition.yaml | Migrates AppDefinition rendering to v1beta11 and adds sidecars templating. |
| .github/workflows/deploy-theia.yml | Makes image tag overrides conditional on non-empty inputs; adds execution_mode for runner selection; bumps CRDS chart version. |
| .github/workflows/deploy-pr.yml | Removes environment selector input; manual dispatch now triggers all environment jobs; passes through execution_mode. |
Comments suppressed due to low confidence (1)
docs/adding-environments.md:189
- The example job snippet hardcodes
deploy_shared_gateway: true, but the actualdeploy-pr.ymljobs now setdeploy_shared_gateway: false. The snippet also omits the newexecution_modeinput that is passed through indeploy-pr.yml, so following these instructions will produce a job that doesn’t match current workflow behavior. Please update the example to reflect the current defaults/inputs (or clearly mark it as an optional variant).
with:
environment: test2
theia_cloud_tag: ${{ inputs.theia_cloud_tag }}
ide_images_tag: ${{ inputs.ide_images_tag }}
helm_chart_tag: ${{ inputs.helm_chart_tag || '' }}
deploy_shared_gateway: true
shared_gateway_values_file: deployments/shared-gateway/values.yaml
shared_gateway_namespace: gateway-system
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Migrates AppDefinitions and the test3 environment toward the v1beta11 sidecar-based architecture needed for validating the PR #70 rollout, and updates deployment workflows/docs to match the new deployment mechanics.
Changes:
- Render AppDefinitions as
theia.cloud/v1beta11and addspec.sidecars[]rendering support in the appdefinitions chart. - Migrate
test3values from legacy langserver options to structured sidecars and align image tags topr-70. - Extend deployment workflows with an
execution_moderunner backend switch and update environment setup/docs accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
charts/theia-appdefinitions/templates/appdefinition.yaml |
Switch AppDefinitions to v1beta11, preserve min/max from existing objects, add sidecar rendering. |
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml |
Update test3 to sidecar-based apps and align images/tags for PR #70 validation. |
.github/workflows/deploy-pr.yml |
Add execution_mode input and pass it to the reusable deploy workflow. |
.github/workflows/deploy-theia.yml |
Add execution_mode input, route runners accordingly, bump CRD chart version, and adjust preload overrides. |
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml |
Add namespace Role/RoleBinding for operator-sidecar pod restart operations. |
deployments/test1.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Pin conversion webhook image for CRD installs. |
deployments/test2.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Pin conversion webhook image for CRD installs. |
deployments/test3.theia-test.artemis.cit.tum.de/theia-crds-helm-values.yml |
Clarify issuer comment and pin conversion webhook image for CRD installs. |
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml |
Pin conversion webhook image for CRD installs. |
deployments/theia.artemis.cit.tum.de/theia-crds-helm-values.yml |
Pin conversion webhook image for CRD installs. |
docs/adding-environments.md |
Update environment-adding guidance to reflect current deploy-pr workflow behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
charts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yaml
Show resolved
Hide resolved
Summary
Main-based migration of test3 deployment to sidecar architecture for PR #70 validation.
Changes in this PR
charts/theia-appdefinitions/templates/appdefinition.yamltheia.cloud/v1beta11spec.sidecars[]supportdeployments/test3.theia-test.artemis.cit.tum.de/values.yamloptions.langserver-imageto structured sidecarspr-70where required for this rollout (operator,service,landing-page,java-17-no-ls,rust-no-ls,langserver-*,defaultImageTag)storageClassName: longhornas intentional for parma-backed test3.github/workflows/deploy-pr.ymlexecution_modedropdown (self-hosted-buildkitorgithub-runners).github/workflows/deploy-theia.ymlexecution_modeinputarc-buildkit-eduide-amd64default,ubuntu-latestfallback)[1..10]whenide_images_tagis setcharts/theia-cloud-combined/templates/rbac-operator-sidecar-pod-restart.yamldeletecollection(intentional for batch sidecar-pod restart cleanup)docs/adding-environments.mdWhy this PR
The previous branch had drift and conflicts. This branch keeps test3 migration scope while aligning deployment behavior and docs.
Dependency / rollout note
conversion-webhook:pr-70pins across env CRD values are intentionally kept during migration validation and will be unpinned/parameterized in a dedicated follow-up after rollout verification.