Add shared cache node helm chart - #16
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:
📝 WalkthroughWalkthroughAdds a new cluster-scoped internal TLS Helm chart and workflow step, integrates a shared cache into the main Theia Cloud helm values and deployments, and introduces monitoring dashboards and ServiceMonitors for the shared cache and Reposilite; documentation and value references updated accordingly. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
f3474a5 to
d459ac9
Compare
| cache: | ||
| hostname: cache.staging.theia-test.artemis.cit.tum.de | ||
| repo: | ||
| hostname: repo.staging.theia-test.artemis.cit.tum.de |
| gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/" | ||
| bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/" |
| enablePush: true | ||
| dependencyCache: | ||
| enabled: true | ||
| url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/" |
|
|
||
| conversion: | ||
| image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest-e431a13 | ||
| image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (6)
charts/theia-cloud-combined/values.yaml:74
charts/theia-cloud-combined/values.yamlconfigures caching viaenableBuildCaching/buildCacheUrl/..., but the rest of this repo’s values (e.g., value-reference-files/theia-cloud-helm-values.yml and environment deployments) uses theoperator.buildCache/operator.dependencyCachestructure. If the underlying theia-cloud chart expects the structured keys, these flags will be ignored and caching won’t actually be enabled/disabled as intended.
enableBuildCaching: false
buildCacheUrl: ""
enableBuildCachePush: false
enableDependencyCaching: false
dependencyCacheUrl: ""
deployments/theia-staging.artemis.cit.tum.de/values.yaml:63
- For the staging environment,
eduide-shared-cache.gateway.*.hostnameis set tocache.staging.theia-test.../repo.staging.theia-test..., but the shared gateway listeners for staging arecache.theia-staging.artemis.cit.tum.deandrepo.theia-staging.artemis.cit.tum.de(and the rest of this values file uses theia-staging.*). With the current hostnames, the gateway routes and certificates are unlikely to match the intended staging domains.
cache:
hostname: cache.staging.theia-test.artemis.cit.tum.de
repo:
hostname: repo.staging.theia-test.artemis.cit.tum.de
deployments/theia-staging.artemis.cit.tum.de/values.yaml:118
theia-cloud.operator.buildCacheURLs for staging currently point tocache.staging.theia-test..., which doesn’t match the staging gateway listener hostname (cache.theia-staging.artemis.cit.tum.de). This mismatch will likely prevent clients from reaching the build cache through the configured endpoint.
buildCache:
enabled: true
gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/"
enablePush: true
deployments/theia-staging.artemis.cit.tum.de/values.yaml:121
theia-cloud.operator.dependencyCache.urlfor staging points torepo.staging.theia-test..., which doesn’t match the staging gateway listener hostname (repo.theia-staging.artemis.cit.tum.de). This will likely make dependency caching fail or hit the wrong environment.
dependencyCache:
enabled: true
url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/"
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml:5
- Pinning controller/webhook images to immutable versions is important for reproducible upgrades. Switching the conversion webhook image to the floating
:latesttag can introduce unexpected changes on the next deploy and make rollbacks harder.
conversion:
image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest
deployments/test2.theia-test.artemis.cit.tum.de/values.yaml:215
- This PR is focused on adding shared-cache support, but this hunk also flips
keycloak.enablefrom true to false for the test2 environment. That’s a significant auth behavior change and seems unrelated to the shared-cache feature; consider moving it to a dedicated PR or documenting why disabling Keycloak is required here.
keycloak:
enable: false
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
deployments/theia-staging.artemis.cit.tum.de/values.yaml:63
- The staging shared-cache gateway hostnames/URLs are pointing to the theia-test domain (cache.staging.theia-test..., repo.staging.theia-test...), but the shared gateway listeners in deployments/shared-gateway/values.yaml are configured for cache.theia-staging.artemis.cit.tum.de / repo.theia-staging.artemis.cit.tum.de. This mismatch will break routing/DNS or certificate expectations for staging.
cache:
hostname: cache.staging.theia-test.artemis.cit.tum.de
repo:
hostname: repo.staging.theia-test.artemis.cit.tum.de
deployments/theia-staging.artemis.cit.tum.de/values.yaml:118
- The build cache URLs for staging also use cache.staging.theia-test..., which does not match the staging gateway hostnames (cache.theia-staging.artemis.cit.tum.de). If the operator uses these URLs, caching will point at the wrong host.
enabled: true
gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/"
enablePush: true
deployments/theia-staging.artemis.cit.tum.de/values.yaml:121
- The dependency cache URL for staging uses repo.staging.theia-test..., which does not match the staging repo gateway hostname (repo.theia-staging.artemis.cit.tum.de).
dependencyCache:
enabled: true
url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/"
charts/theia-cloud-combined/values.yaml:74
- This chart's default values use operator flags (enableBuildCaching/buildCacheUrl/etc), but the repo's deployment and reference values use operator.buildCache / operator.dependencyCache objects. Keeping both schemas makes the combined chart defaults misleading and likely non-functional for users copying values.yaml.
enableBuildCaching: false
buildCacheUrl: ""
enableBuildCachePush: false
enableDependencyCaching: false
dependencyCacheUrl: ""
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml:5
- Switching the conversion webhook image tag to ':latest' makes deployments non-reproducible and can introduce breaking changes unexpectedly. Prefer a pinned tag (or digest) as before.
conversion:
image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest
| keycloak: | ||
| enable: true | ||
| enable: false |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (4)
charts/theia-cloud-combined/values.yaml:74
- The operator cache configuration here uses legacy keys (enableBuildCaching/buildCacheUrl/...) that don’t match the buildCache/dependencyCache structure used elsewhere in this repo (e.g., value-reference-files/theia-cloud-helm-values.yml). As a result, these values are likely ignored by the theia-cloud chart/operator and caching won’t actually be configurable via this chart.
enableBuildCaching: false
buildCacheUrl: ""
enableBuildCachePush: false
enableDependencyCaching: false
dependencyCacheUrl: ""
deployments/theia-staging.artemis.cit.tum.de/values.yaml:63
- These shared-cache gateway hostnames don’t match the staging listeners configured in deployments/shared-gateway/values.yaml (which use cache.theia-staging.artemis.cit.tum.de / repo.theia-staging.artemis.cit.tum.de). With the current values, the Gateway listener sectionName and hostnames won’t align, so routing/certificates are likely to break.
cache:
hostname: cache.staging.theia-test.artemis.cit.tum.de
repo:
hostname: repo.staging.theia-test.artemis.cit.tum.de
deployments/theia-staging.artemis.cit.tum.de/values.yaml:121
- The operator cache URLs here use cache.staging.theia-test.artemis.cit.tum.de / repo.staging.theia-test.artemis.cit.tum.de, but the shared gateway listeners for staging are configured for cache.theia-staging.artemis.cit.tum.de / repo.theia-staging.artemis.cit.tum.de. This mismatch will cause clients to target a hostname that the gateway/certs likely don’t serve.
enabled: true
gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/"
enablePush: true
dependencyCache:
enabled: true
url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/"
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml:5
- Using the mutable :latest tag for the conversion webhook makes staging deployments non-reproducible and can introduce unplanned changes. This file previously pinned a specific build tag; it’s safer to keep an immutable version (tag or digest).
image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
deployments/theia-staging.artemis.cit.tum.de/values.yaml:63
- The shared-cache gateway hostnames point to the theia-test zone (cache.staging.theia-test...) while the rest of this deployment uses the theia-staging.artemis.cit.tum.de domain. This will cause the Gateway parentRefs (staging-cache/staging-repo) to route to hostnames that aren’t covered by the shared-gateway listeners for staging.
cache:
hostname: cache.staging.theia-test.artemis.cit.tum.de
repo:
hostname: repo.staging.theia-test.artemis.cit.tum.de
deployments/theia-staging.artemis.cit.tum.de/values.yaml:118
- operator.buildCache URLs are configured for cache.staging.theia-test..., but the staging gateway listener is cache.theia-staging.artemis.cit.tum.de. These URLs should match the shared-cache gateway hostname, otherwise build cache traffic will go to the wrong domain.
buildCache:
enabled: true
gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/"
enablePush: true
deployments/theia-staging.artemis.cit.tum.de/values.yaml:121
- operator.dependencyCache.url is configured for repo.staging.theia-test..., but the staging gateway listener is repo.theia-staging.artemis.cit.tum.de. This mismatch will send dependency downloads to the wrong domain.
dependencyCache:
enabled: true
url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/"
deployments/theia.artemis.cit.tum.de/values.yaml:117
- buildCache.enablePush is enabled while eduide-shared-cache auth is disabled, which allows unauthenticated clients to write into the shared build cache (risk of cache poisoning / untrusted writes). If this must stay unauthenticated, push should be disabled.
buildCache:
enabled: true
gradleUrl: "https://cache.theia.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.theia.artemis.cit.tum.de/bazel/"
enablePush: true
deployments/theia-staging.artemis.cit.tum.de/theia-crds-helm-values.yml:5
- The conversion webhook image is set to the mutable ':latest' tag. This reduces deployment reproducibility and can lead to unexpected changes when the image is updated upstream. Prefer pinning to an immutable tag (release/commit) or digest.
conversion:
image: ghcr.io/eduide/eduide-cloud/conversion-webhook:latest
| eduide-shared-cache: | ||
| enabled: true | ||
| auth: | ||
| enabled: false |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (2)
deployments/theia-staging.artemis.cit.tum.de/values.yaml:63
- The shared-cache gateway hostnames for the staging deployment use the
*.staging.theia-test.artemis.cit.tum.dedomain, but the rest of this deployment (hosts configuration) usestheia-staging.artemis.cit.tum.de, and the shared gateway listeners are configured forcache.theia-staging.artemis.cit.tum.de/repo.theia-staging.artemis.cit.tum.de. This mismatch will route cache/repo traffic to the wrong hostnames.
cache:
hostname: cache.staging.theia-test.artemis.cit.tum.de
repo:
hostname: repo.staging.theia-test.artemis.cit.tum.de
deployments/theia-staging.artemis.cit.tum.de/values.yaml:121
- The operator's build/dependency cache URLs point to
cache.staging.theia-test.artemis.cit.tum.de/repo.staging.theia-test.artemis.cit.tum.de, but the staging environment in this repo is configured undertheia-staging.artemis.cit.tum.de(and the shared gateway exposes cache/repo on that domain). With the current values, clients will use hostnames that likely won't resolve to the shared gateway listeners.
buildCache:
enabled: true
gradleUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/gradle/"
bazelUrl: "https://cache.staging.theia-test.artemis.cit.tum.de/bazel/"
enablePush: true
dependencyCache:
enabled: true
url: "https://repo.staging.theia-test.artemis.cit.tum.de/releases/"
Summary by CodeRabbit