fix(test3): point landing page image to eduidec repository#83
fix(test3): point landing page image to eduidec repository#83CodeByNikolas wants to merge 45 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
📝 WalkthroughWalkthroughThis PR adds a 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. Important Merge conflicts detected (Beta)
✨ 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
Updates the Test3 environment configuration and deployment workflows, primarily to align the landing page image repository with the new ghcr.io/eduide/eduidec-landing-page location and to introduce an optional “clean install” deployment mode.
Changes:
- Update
test3landing page image toghcr.io/eduide/eduidec-landing-page:latestand adjust several other Test3 Helm values. - Add a
clean_installinput to deployment workflows and a namespace-scoped purge step before Helm install. - Add stricter shell failure behavior (
set -euo pipefail) to the cluster-wide monitoring install step.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| deployments/test3.theia-test.artemis.cit.tum.de/values.yaml | Updates Test3 landing page image repo, but also changes preloading list ordering/content and other environment settings. |
| .github/workflows/deploy-theia.yml | Adds clean_install input and a namespace purge step; also adds set -euo pipefail to monitoring step. |
| .github/workflows/deploy-pr.yml | Exposes and forwards clean_install from manual dispatch to the reusable deploy workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key: ADMIN_API_TOKEN | ||
|
|
||
| preloading: | ||
| images: |
There was a problem hiding this comment.
theia-cloud.preloading.images no longer includes the landing page image as the first entry. This repo’s deploy workflow assumes preloading.images[0] is the landing page (and overrides indices 1..10 for IDE images), so removing it will break tag overrides and can shift/incorrectly override preload images. Add ghcr.io/eduide/eduidec-landing-page:... back as images[0] (and keep the expected ordering) or update the deploy workflow to not rely on fixed indices for this environment.
| images: | |
| images: | |
| - ghcr.io/eduide/eduidec-landing-page:pr-100 |
| @@ -83,7 +78,7 @@ theia-cloud: | |||
|
|
|||
| 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 | |||
There was a problem hiding this comment.
The comment says “does not support mutliple apps” — “mutliple” is misspelled. Please correct it to “multiple”.
| # 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 | |
| # We use the try now page as landing page since the default does not support multiple apps -> https://github.com/eclipsesource/theia-cloud/discussions/301 |
| @@ -56,24 +56,19 @@ theia-cloud: | |||
| ephemeral-storage: "256Mi" | |||
|
|
|||
| service: | |||
| image: ghcr.io/eduide/eduide-cloud/service:pr-70 | |||
| image: ghcr.io/eduide/eduide-cloud/service:pr-100 | |||
| adminApiTokenSecret: | |||
There was a problem hiding this comment.
This PR is described as only updating the test3 landing page image repository, but this file also changes operator/service tags, preloading images/tags, Keycloak enablement, AppDefinition scaling, and other landing page settings. Please either update the PR title/description to reflect the broader scope or split these unrelated changes into separate PRs to keep review and rollout risk manageable.
| --set wildcardTLSSecret.key="$(cat shared-gateway-wildcard.key)" | ||
|
|
||
| - name: Clean install - namespace-scoped resource purge | ||
| if: inputs.clean_install && github.event_name == 'workflow_dispatch' |
There was a problem hiding this comment.
The clean-install purge step will never run in this reusable workflow: when deploy-theia.yml is invoked via workflow_call, github.event_name is workflow_call, not workflow_dispatch, so this if: condition is always false. Rely on inputs.clean_install alone (the caller already gates it to manual dispatch) or pass an explicit “triggered_by_dispatch” input if you need extra protection.
| if: inputs.clean_install && github.event_name == 'workflow_dispatch' | |
| if: inputs.clean_install |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
deployments/test3.theia-test.artemis.cit.tum.de/values.yaml (2)
141-144:java-17-latestlacks sidecars - is this intentional?The new
java-17-latestapp definition uses the full IDE image (java-17) without any sidecar configuration, whilejava-17-no-lsandrust-no-lshave langserver sidecars. This appears to be the "monolithic" IDE variant vs. the sidecar architecture variant.Consider adding a brief comment above this app definition to clarify that it intentionally runs without a separate language server sidecar.
🤖 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 141 - 144, Add a short clarifying comment above the java-17-latest app definition to state that this entry is the monolithic IDE image and intentionally does not include a language-server sidecar (unlike java-17-no-ls and rust-no-ls); update the comment near the block containing "name: java-17-latest" so reviewers know the omission of sidecars is deliberate.
116-126: Consider adding a comment explaining why Keycloak is disabled in test3.Keycloak configuration diverges across test environments: test1 and test2 both enable Keycloak with realm "Test" and clientId "theia-test", while test3 disables it with empty credentials. This appears intentional for an unauthenticated test environment, but adding an inline comment would clarify this design decision for future maintainers.
🤖 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 116 - 126, Add an inline comment near the keycloak block explaining why Keycloak is disabled for this environment (keycloak.enable: false) and that authUrl, realm, and clientId are left empty intentionally for an unauthenticated test environment; reference the keycloak keys (enable, authUrl, realm, clientId) and note that test1/test2 use realm "Test" and clientId "theia-test" so maintainers understand the divergence and when to populate those fields if Keycloak is re-enabled..github/workflows/deploy-theia.yml (2)
296-300: Minor: Consider usingkubectl api-resourcesfiltering directly.The current approach fetches all namespaced resources then filters with grep. You could simplify by targeting known CRD names directly if they're stable.
💡 Alternative: Target known CRDs directly
# If the CRD names are known and stable, target them directly: for kind in sessions workspaces; do kubectl delete "$kind" --all -n "$NAMESPACE" --ignore-not-found=true || true done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-theia.yml around lines 296 - 300, Replace the generic read-and-grep loop that iterates over RESOURCE_LIST with a targeted deletion loop that iterates the known CRD kinds (e.g., "sessions" and "workspaces") and calls kubectl delete for each kind in the NAMESPACE; specifically, remove the while IFS= read -r resource; ... done <<< "$RESOURCE_LIST" block and implement a simple loop over the stable kinds that invokes kubectl delete --all -n "$NAMESPACE" --ignore-not-found=true (keeping the || true behavior) so you directly target CRDs instead of filtering all namespaced resources.
285-316: Review the clean install purge step for safety and completeness.The implementation has good safeguards:
- Double-gated condition (
inputs.clean_install && github.event_name == 'workflow_dispatch')- Uses
--ignore-not-found=trueto avoid failures on missing resources- Proper ordering: sessions → appdefinitions → workloads → wait → PVCs
A few observations:
Regex pattern may be fragile: The grep pattern
^sessions(\.|$)|^session(\.|$)|...matches resource names starting with "session" or "workspace", but CRD names could vary (e.g.,theia.cloud/sessionsvssessions.theia.cloud).Consider logging what's being deleted: For audit purposes, listing resources before deletion would help troubleshoot issues.
120s timeout may be insufficient: For environments with many pods, termination could take longer.
🔍 Optional: Add resource listing before deletion for audit trail
# 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 + echo "Deleting $resource resources in $NAMESPACE..." + kubectl get "$resource" -n "$NAMESPACE" --no-headers 2>/dev/null || true kubectl delete "$resource" --all -n "$NAMESPACE" --ignore-not-found=true || true fi done <<< "$RESOURCE_LIST"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-theia.yml around lines 285 - 316, The session/workspace resource purge uses a fragile grep pattern and lacks logging and configurability: update the RESOURCE_LIST filtering to robustly match CRD short-names and namespaced API resources that contain "session" or "workspace" either as the short name or the resource name (handle forms like "sessions.theia.cloud" and "theia.cloud/sessions") by extracting both the name and the API name from kubectl api-resources output and matching on either field (refer to RESOURCE_LIST and the grep/filter logic in the loop), add a pre-deletion echo or log of the exact resources that will be deleted for audit, and replace the hard-coded kubectl wait --for=delete pod --all -n "$NAMESPACE" --timeout=120s with a configurable timeout variable (e.g., CLEAN_WAIT_TIMEOUT) defaulting to a larger value so environments with many pods have more time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/deploy-theia.yml:
- Around line 296-300: Replace the generic read-and-grep loop that iterates over
RESOURCE_LIST with a targeted deletion loop that iterates the known CRD kinds
(e.g., "sessions" and "workspaces") and calls kubectl delete for each kind in
the NAMESPACE; specifically, remove the while IFS= read -r resource; ... done
<<< "$RESOURCE_LIST" block and implement a simple loop over the stable kinds
that invokes kubectl delete --all -n "$NAMESPACE" --ignore-not-found=true
(keeping the || true behavior) so you directly target CRDs instead of filtering
all namespaced resources.
- Around line 285-316: The session/workspace resource purge uses a fragile grep
pattern and lacks logging and configurability: update the RESOURCE_LIST
filtering to robustly match CRD short-names and namespaced API resources that
contain "session" or "workspace" either as the short name or the resource name
(handle forms like "sessions.theia.cloud" and "theia.cloud/sessions") by
extracting both the name and the API name from kubectl api-resources output and
matching on either field (refer to RESOURCE_LIST and the grep/filter logic in
the loop), add a pre-deletion echo or log of the exact resources that will be
deleted for audit, and replace the hard-coded kubectl wait --for=delete pod
--all -n "$NAMESPACE" --timeout=120s with a configurable timeout variable (e.g.,
CLEAN_WAIT_TIMEOUT) defaulting to a larger value so environments with many pods
have more time.
In `@deployments/test3.theia-test.artemis.cit.tum.de/values.yaml`:
- Around line 141-144: Add a short clarifying comment above the java-17-latest
app definition to state that this entry is the monolithic IDE image and
intentionally does not include a language-server sidecar (unlike java-17-no-ls
and rust-no-ls); update the comment near the block containing "name:
java-17-latest" so reviewers know the omission of sidecars is deliberate.
- Around line 116-126: Add an inline comment near the keycloak block explaining
why Keycloak is disabled for this environment (keycloak.enable: false) and that
authUrl, realm, and clientId are left empty intentionally for an unauthenticated
test environment; reference the keycloak keys (enable, authUrl, realm, clientId)
and note that test1/test2 use realm "Test" and clientId "theia-test" so
maintainers understand the divergence and when to populate those fields if
Keycloak is re-enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23c7075a-52b7-40c0-b62d-bd3a099c7c0c
📒 Files selected for processing (3)
.github/workflows/deploy-pr.yml.github/workflows/deploy-theia.ymldeployments/test3.theia-test.artemis.cit.tum.de/values.yaml
Summary
ghcr.io/eduide/eduide-cloud/landing-page:latesttoghcr.io/eduide/eduidec-landing-page:latestWhy
The landing-page repository now builds/pushes images under
eduide/eduidec-landing-page. test3 still referenced the oldeduide-cloud/landing-pagepath and would not pick up new landing fixes.Summary by CodeRabbit
New Features
clean_installoption for deployments that conditionally purges resources before installation/upgrade.Chores