fix: disable keycloak on test3 for benchmark runs#81
fix: disable keycloak on test3 for benchmark runs#81CodeByNikolas wants to merge 65 commits intomainfrom
Conversation
- Update test3 deployment to use ghcr.io/ls1intum/edutheia-landing-page - Add landing_page_tag input parameter to deployment workflows - Separate landing page versioning from theia-cloud components - Update preloading configuration for new landing page image
Update chart dependency to version 1.3.0-next.4 which includes the footerLinks feature. This enables footer link customization in the landing page ConfigMap.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ate landing page image to ghcr.io/EduIDE/eduidec-landing-page
- operator/service: ghcr.io/eduide/eduide-cloud/{operator,service} (from EduIDE-Cloud repo)
- landing page: ghcr.io/EduIDE/eduidec-landing-page (from EduIDE-Landing-Page repo)
- IDE images (java-17, c, rust, etc.): ghcr.io/eduide/eduide/... (from EduIDE repo) - unchanged
… deployment workflows
…into feat/landing-page-integration-refresh
|
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:
📝 WalkthroughWalkthroughThis pull request introduces a clean-install capability to deployment workflows, enabling manual snapshot and restoration of AppDefinition instances. It updates image tags across deployment configurations, disables Keycloak authentication, adds a new Java 17 app definition, and hardens shell execution with improved error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| landingPage: | ||
| # We use the try now page as landing page since the default does not support mutliple apps -> https://github.com/eclipsesource/theia-cloud/discussions/301 | ||
| image: ghcr.io/eduide/eduide-cloud/landing-page:pr-70 | ||
| image: ghcr.io/eduide/eduide-cloud/landing-page:latest |
There was a problem hiding this comment.
landingPage.image is set to ghcr.io/eduide/eduide-cloud/landing-page:latest, but other environments and the deployment workflow overrides use ghcr.io/eduide/eduidec-landing-page for the try-now landing page. This mismatch can lead to test3 serving a different landing page than intended (and behaving differently from tag overrides). Align the repo/image name with the other deployments/workflow defaults to avoid surprises.
| image: ghcr.io/eduide/eduide-cloud/landing-page:latest | |
| image: ghcr.io/eduide/eduidec-landing-page:latest |
| keycloak: | ||
| enable: true | ||
| enable: false | ||
|
|
||
| # -- Key cloak auth URL. Only has to be specified when enable: true | ||
| authUrl: "https://keycloak.ase.in.tum.de/" | ||
|
|
||
| # -- The Keycloak Realm. Only has to be specified when enable: true | ||
| realm: "Test" | ||
| realm: "" | ||
|
|
||
| # -- The client-id. Only has to be specified when enable: true | ||
| clientId: "theia-test" | ||
| clientId: "" | ||
|
|
||
| theia-appdefinitions: | ||
| defaultImageTag: pr-70 | ||
| defaultImageTag: pr-130 | ||
| apps: | ||
| - name: java-17-no-ls | ||
| image: ghcr.io/eduide/eduide/java-17-no-ls | ||
| minInstances: 1 | ||
| minInstances: 3 |
There was a problem hiding this comment.
PR title/description says the only change is disabling Keycloak for test3, but this values file also changes multiple unrelated settings (operator/service image tags, preloading image set, landing page config, ephemeralStorage, appdefinition scaling, and adds a new app). Either narrow the commit to just the Keycloak toggle or update the PR description/scope so reviewers and release notes reflect the full impact.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| operator: | ||
| image: ghcr.io/eduide/eduide-cloud/operator:pr-70 | ||
| image: ghcr.io/eduide/eduide-cloud/operator:pr-100 |
There was a problem hiding this comment.
PR description says Keycloak should be disabled for test3 and other values unchanged, but this values file still has theia-cloud.keycloak.enable: true and also changes component image tags. Either update this file to actually set theia-cloud.keycloak.enable: false (and revert unrelated changes) or update the PR title/description to match what is being changed.
| - ghcr.io/eduide/eduide/java-17:pr-130 | ||
| - ghcr.io/eduide/eduide/rust:pr-130 | ||
| - ghcr.io/eduide/eduide/java-17-no-ls:pr-130 | ||
| - ghcr.io/eduide/eduide/rust-no-ls:pr-130 | ||
| - ghcr.io/eduide/eduide/langserver-java:pr-130 | ||
| - ghcr.io/eduide/eduide/langserver-rust:pr-130 | ||
|
|
There was a problem hiding this comment.
theia-cloud.preloading.images no longer includes the landing page image at index 0. This breaks the convention used elsewhere in the repo and can also conflict with the deploy workflow which overrides IDE preload images starting at index 1 (and assumes index 0 is the landing page). Put ghcr.io/eduide/eduidec-landing-page:... back as images[0] (or adjust the workflow’s index mapping accordingly).
| - ghcr.io/eduide/eduide/java-17:pr-130 | |
| - ghcr.io/eduide/eduide/rust:pr-130 | |
| - ghcr.io/eduide/eduide/java-17-no-ls:pr-130 | |
| - ghcr.io/eduide/eduide/rust-no-ls:pr-130 | |
| - ghcr.io/eduide/eduide/langserver-java:pr-130 | |
| - ghcr.io/eduide/eduide/langserver-rust:pr-130 | |
| - ghcr.io/eduide/eduidec-landing-page:pr-100 | |
| - ghcr.io/eduide/eduide/java-17:pr-130 | |
| - ghcr.io/eduide/eduide/rust:pr-130 | |
| - ghcr.io/eduide/eduide/java-17-no-ls:pr-130 | |
| - ghcr.io/eduide/eduide/rust-no-ls:pr-130 | |
| - ghcr.io/eduide/eduide/langserver-java:pr-130 | |
| - ghcr.io/eduide/eduide/langserver-rust:pr-130 |
| NAMESPACE="${{ vars.NAMESPACE }}" | ||
|
|
||
| # Delete active session resources first if these CRDs exist | ||
| RESOURCE_LIST="$(kubectl api-resources --namespaced=true -o name)" | ||
| while IFS= read -r resource; do | ||
| if echo "$resource" | grep -Eq '^sessions(\.|$)|^session(\.|$)|^workspaces(\.|$)|^workspace(\.|$)'; then | ||
| kubectl delete "$resource" --all -n "$NAMESPACE" --ignore-not-found=true || true | ||
| fi | ||
| done <<< "$RESOURCE_LIST" | ||
|
|
||
| # Delete AppDefinitions for a full clean slate | ||
| kubectl delete appdefinitions --all -n "$NAMESPACE" --ignore-not-found=true | ||
|
|
||
| # Delete namespace workloads | ||
| kubectl delete deployments --all -n "$NAMESPACE" --ignore-not-found=true |
There was a problem hiding this comment.
This step performs destructive deletes based on NAMESPACE="${{ vars.NAMESPACE }}" but doesn’t validate that the namespace is non-empty / expected. With set -euo pipefail, a misconfigured or empty namespace could lead to deleting resources in the runner’s default namespace. Add an explicit safety check (e.g., fail fast if NAMESPACE is empty or equals default, and echo the namespace before deleting).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| landingPage: | ||
| # We use the try now page as landing page since the default does not support mutliple apps -> https://github.com/eclipsesource/theia-cloud/discussions/301 | ||
| image: ghcr.io/eduide/eduide-cloud/landing-page:pr-70 | ||
| image: ghcr.io/eduide/eduidec-landing-page:latest |
There was a problem hiding this comment.
landingPage.image is set to :latest, which makes the test3 landing page (and benchmark entrypoint) non-deterministic across runs. If the goal is deterministic benchmark runs, consider pinning this to an immutable tag (or digest) that matches the benchmark baseline.
| image: ghcr.io/eduide/eduidec-landing-page:latest | |
| image: ghcr.io/eduide/eduidec-landing-page:pr-131 |
| - name: java-17-no-ls | ||
| image: ghcr.io/eduide/eduide/java-17-no-ls | ||
| minInstances: 1 | ||
| minInstances: 3 | ||
| maxInstances: 20 | ||
| sidecars: | ||
| - name: langserver | ||
| image: ghcr.io/eduide/eduide/langserver-java | ||
| port: 5000 | ||
| languages: [java] | ||
| mountWorkspace: true | ||
| - name: java-17-latest | ||
| image: ghcr.io/eduide/eduide/java-17 | ||
| minInstances: 3 | ||
| maxInstances: 20 |
There was a problem hiding this comment.
The theia-appdefinitions chart preserves existing AppDefinition minInstances/maxInstances from the live resource (via lookup) on subsequent Helm upgrades, so increasing these values here won’t take effect in an already-deployed namespace unless the AppDefinition resources are deleted first (or you run a clean install that removes them). If the intent is to change scaling for an existing test3 install, you may need to pair this change with an explicit AppDefinition purge/uninstall step.
| url: "https://aet.cit.tum.de/" | ||
|
|
||
| keycloak: | ||
| enable: true | ||
| enable: false |
There was a problem hiding this comment.
The PR description says the only functional change for test3 is disabling Keycloak and otherwise keeping values unchanged, but this file also changes operator/service image tags, preload image list, landing page image, ephemeralStorage, app labels, and appdefinitions scaling/app list. If those are unintended, it would be safer to revert them; if intended, the PR description should be updated to match the actual scope.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| image: ghcr.io/eduide/eduide-cloud/operator:latest-ba732d8 | ||
| # Production keeps Ceph RBD and does not configure sidecars. | ||
| # Disable eager-start so the operator stays on the workspace-backed RWO path | ||
| # instead of creating prewarmed RWX PVCs that do not fit csi-rbd-sc. | ||
| eagerStart: false |
There was a problem hiding this comment.
The PR is scoped (per title/description) to disabling Keycloak on test3, but this file also pins production operator/service images and enables the service: block. Please update the PR description to reflect these additional production deployment changes, or move them into a separate PR to reduce rollout risk.
| image: ghcr.io/eduide/eduide-cloud/operator:pr-101 | ||
| eagerStart: false | ||
| replicas: 1 | ||
| sessionsPerUser: 10 | ||
| # Test3 runs on the parma cluster where Longhorn is the default storage backend. |
There was a problem hiding this comment.
PR description says test3 should only disable Keycloak and keep other values unchanged, but this hunk also changes operator image/tag and eagerStart (and further down other settings too). Please either update the PR description/scope (or split into separate PRs) or revert unrelated test3 config changes so the intent is clear and reviewable.
| eagerStart: false | ||
| replicas: 1 | ||
| sessionsPerUser: 10 | ||
| # Test3 runs on the parma cluster where Longhorn is the default storage backend. |
There was a problem hiding this comment.
The comment says Test3 uses Longhorn as the default storage backend, but storageClassName is set to csi-rbd-sc. Either update the comment to match the new backend, or revert storageClassName to longhorn if that’s still the correct class for the parma cluster.
| # Test3 runs on the parma cluster where Longhorn is the default storage backend. | |
| # Test3 runs on the parma cluster using the csi-rbd-sc storage class. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Preload list indices 0–10 must match deploy-theia.yml --set overrides when tags are passed. | ||
| # Index 11 is oauth2-proxy (distroless); the workflow does not override it. | ||
| preloading: | ||
| images: | ||
| - ghcr.io/eduide/eduidec-landing-page:latest | ||
| - ghcr.io/eduide/eduide/java-17:latest |
There was a problem hiding this comment.
theia-cloud.preloading.images no longer matches the documented fixed index layout (0=landing page, 1–10 IDE images, 11=oauth2-proxy) that deploy-theia.yml relies on via --set theia-cloud.preloading.images[N]=.... With the current shortened list starting at java-17, tag overrides can end up writing sparse arrays / wrong indices and break preloading expectations. Restore the full indexed list (including landing page at index 0 and oauth2-proxy at index 11), or update the workflow and the in-file comment together so they remain consistent.
| url: "https://aet.cit.tum.de/" | ||
|
|
||
| keycloak: | ||
| enable: true | ||
| enable: false |
There was a problem hiding this comment.
PR description says only Keycloak should be disabled for test3 and all other test3 values kept unchanged, but this file also changes operator/service image tags, eagerStart, preloading image list, landingPage ephemeralStorage, and appdefinition scaling/default tags. Please either narrow the diff to only the Keycloak toggle, or update the PR description (and ideally split into separate PRs) so reviewers can validate the additional behavioral/operational changes explicitly.
| - name: java-17-no-ls | ||
| image: ghcr.io/eduide/eduide/java-17-no-ls | ||
| minInstances: 1 | ||
| minInstances: 5 | ||
| maxInstances: 20 |
There was a problem hiding this comment.
Bumping minInstances from 1 to 5 (and adding maxInstances: 20) for app definitions will increase baseline resource usage and can change how quickly/consistently workspaces start (especially relevant for benchmark runs). Confirm these scaling defaults are intentional for test3; if not, revert to the previous values or document why the higher prewarm is required.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| operator: | ||
| image: ghcr.io/eduide/eduide-cloud/operator:latest-dfe0d09 | ||
| #eagerStart: false | ||
| image: ghcr.io/eduide/eduide-cloud/operator:latest-ba732d8 | ||
| # Production keeps Ceph RBD and does not configure sidecars. | ||
| # Disable eager-start so the operator stays on the workspace-backed RWO path | ||
| # instead of creating prewarmed RWX PVCs that do not fit csi-rbd-sc. | ||
| eagerStart: false | ||
| replicas: 3 | ||
| sessionsPerUser: 10 | ||
| storageClassName: csi-rbd-sc | ||
|
|
||
| service: | ||
| image: ghcr.io/eduide/eduide-cloud/service:latest-dfe0d09 | ||
| image: ghcr.io/eduide/eduide-cloud/service:latest-ba732d8 |
There was a problem hiding this comment.
This file appears to be the production deployment (deployments/theia.artemis.cit.tum.de), but the PR’s title/description only mention disabling Keycloak for test3. Updating production operator/service image tags and changing eagerStart behavior is a high-impact change; please move these production changes to a dedicated PR (or update the PR description and ensure production rollout implications are explicitly intended).
| # Preload list indices 0–10 must match deploy-theia.yml --set overrides when tags are passed. | ||
| # Index 11 is oauth2-proxy (distroless); the workflow does not override it. | ||
| preloading: | ||
| images: | ||
| - ghcr.io/eduide/eduidec-landing-page:latest | ||
| - ghcr.io/eduide/eduide/java-17:latest | ||
| - ghcr.io/eduide/eduide/c:latest | ||
| - ghcr.io/eduide/eduide/javascript:latest | ||
| - ghcr.io/eduide/eduide/ocaml:latest | ||
| - ghcr.io/eduide/eduide/rust:latest | ||
| - ghcr.io/eduide/eduide/python:latest | ||
| - ghcr.io/eduide/eduide/java-17-no-ls:pr-70 | ||
| - ghcr.io/eduide/eduide/rust-no-ls:pr-70 | ||
| - ghcr.io/eduide/eduide/langserver-java:pr-70 | ||
| - ghcr.io/eduide/eduide/langserver-rust:pr-70 | ||
| - image: quay.io/oauth2-proxy/oauth2-proxy:v7.12.0 | ||
| args: ["--version"] | ||
| - ghcr.io/eduide/eduide/java-17-no-ls:latest | ||
| - ghcr.io/eduide/eduide/rust-no-ls:latest | ||
| - ghcr.io/eduide/eduide/langserver-java:latest | ||
| - ghcr.io/eduide/eduide/langserver-rust:latest |
There was a problem hiding this comment.
theia-cloud.preloading.images no longer includes the landing page at index 0 (and the list is shorter than the workflow’s expected 0–10/11 layout). This will misalign .github/workflows/deploy-theia.yml --set theia-cloud.preloading.images[0..10]=... overrides when tags are provided, causing the wrong images to be overridden/preloaded. Restore the full indexed list (landing at [0], IDE/no-ls/langserver at [1..10], and either keep oauth2-proxy at [11] or update the surrounding comments + workflow consistently).
| - name: java-17-no-ls | ||
| image: ghcr.io/eduide/eduide/java-17-no-ls | ||
| minInstances: 1 | ||
| minInstances: 5 | ||
| maxInstances: 20 | ||
| sidecars: | ||
| - name: langserver | ||
| image: ghcr.io/eduide/eduide/langserver-java | ||
| port: 5000 | ||
| languages: [java] | ||
| mountWorkspace: true | ||
| - name: java-17-latest | ||
| image: ghcr.io/eduide/eduide/java-17 | ||
| minInstances: 5 | ||
| maxInstances: 20 | ||
| - name: rust-no-ls | ||
| image: ghcr.io/eduide/eduide/rust-no-ls | ||
| minInstances: 1 | ||
| minInstances: 5 | ||
| maxInstances: 20 |
There was a problem hiding this comment.
minInstances is increased from 1 to 5 (and maxInstances added at 20) for multiple app definitions. This can significantly increase baseline resource usage in the test3 namespace. If the goal is only benchmark runs with unauthenticated access, consider keeping scaling unchanged here (or justify these new scaling defaults and confirm the cluster has capacity).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: java-17-no-ls | ||
| image: ghcr.io/eduide/eduide/java-17-no-ls | ||
| minInstances: 1 | ||
| minInstances: 5 | ||
| maxInstances: 200 | ||
| sidecars: |
There was a problem hiding this comment.
theia-appdefinitions scaling defaults are changed significantly here (e.g., minInstances: 5 / maxInstances: 200). Even though the chart preserves live scaling after initial creation, these values still affect new/clean installs and can materially increase resource usage in test3. If the intent is only unauthenticated access for benchmarks, consider reverting these scaling/appdefinition changes or documenting why they’re required.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| operator: | ||
| image: ghcr.io/eduide/eduide-cloud/operator:latest-e431a13 | ||
| #eagerStart: false | ||
| # Production keeps Ceph RBD and does not configure sidecars. | ||
| # Disable eager-start so the operator stays on the workspace-backed RWO path | ||
| # instead of creating prewarmed RWX PVCs that do not fit csi-rbd-sc. | ||
| eagerStart: false | ||
| replicas: 3 |
There was a problem hiding this comment.
The PR description indicates only a test3 Keycloak toggle, but this production values file now enables operator.eagerStart: false (previously commented). Please update the PR description/scope or split this change, since it affects production behavior and deserves separate review/rollout planning.
Summary
test3deployment by settingtheia-cloud.keycloak.enabletofalseWhy
Summary by CodeRabbit