Add integrations-hub Helm chart for Kubernetes deployment#614
Conversation
- Add integrations-hub chart with deployment, service, and service account templates - Configure environment variables for Next.js, PostgreSQL, Auth.js, and OpenHands auth - Add health/ready probe endpoints at /api/health and /api/ready - Support both GCP Cloud SQL and in-cluster PostgreSQL configurations - Add integrations-hub as optional dependency in openhands chart - Add ingress template for integrations-hub in openhands chart - Update GitHub workflows to include integrations-hub chart publishing Co-authored-by: openhands <openhands@all-hands.dev>
Changes for openhands-auth-python branch: - Change default port from 3000 to 8000 - Update health check paths from /api/health to /health - Replace Node.js/NextAuth env vars with INTEGRATIONS_HUB_* prefix - Add OpenHands cookie auth configuration - Add CORS and DB pool settings - Remove auth.js/NextAuth configuration Co-authored-by: openhands <openhands@all-hands.dev>
Run alembic migrations before starting the main container to ensure database tables exist. This follows the same pattern as the automation service.
Default INTEGRATIONS_HUB_APP_ADMIN_EMAILS to: chuck@openhands.dev,chuck@all-hands.dev,graham@openhands.dev,graham@all-hands.dev Co-authored-by: openhands <openhands@all-hands.dev>
…kend The chart previously exported a Next.js-era `INTEGRATIONS_HUB_*` env var prefix plus separate INTEGRATIONS_HUB_DB_HOST/PORT/USER/PASS/NAME values, none of which the FastAPI backend on the development branch actually reads. Rewrite `_env.yaml` to match what backend/app/config.py and oauth_flow.py look for so the pod can actually start and reach Postgres: - Use the canonical `INTHUB_*` prefix (with legacy unprefixed fallbacks the backend already accepts) for: POSTGRES_URL, OPENHANDS_BASE_URL, OPENHANDS_AUTH_COOKIE_NAME, INTERNAL_AUTH_SECRET, CRON_SECRET, CREDENTIAL_ENCRYPTION_KEY, DISABLE_AUTH. - Build a single `INTHUB_POSTGRES_URL` from the configured DB host/port/user/name plus the password secret using Kubernetes $(VAR_NAME) expansion. `database.url` is a new opt-in escape hatch for callers that want to supply a pre-formed URL. - Use the unprefixed `APP_ADMIN_EMAILS` (per backend/README.md, the prefixed form is JSON-decoded by the SDK env parser and would require list syntax). - Add optional `AUTH_REDIRECT_PROXY_URL` / `AUTH_URL` for the OAuth preview-deployment proxy that backend/app/oauth_flow.py honours. - Drop unused env vars (HOST, SERVER_PORT, LOG_LEVEL, BASE_URL, GCP_*, CORS_ORIGINS) the FastAPI app never reads. Also fix the health probes -- the FastAPI app exposes `/api/health` and has no `/ready` endpoint, so the previous `/health` and `/ready` probes would fail. Reuse `/api/health` for startup/liveness/readiness. Finally, correct the umbrella chart's stale `service.port: 3000` (Next.js) to `8000` (FastAPI `EXPOSE 8000`), mirror the new optional fields, and rename the in-cluster wait-for-postgres init container's PGPASSWORD reference to $INTHUB_DB_PASS so it lines up with the renamed env var. Verified with helm template + helm lint against several value matrices (defaults, database.url override, OAuth proxy, internal auth secret, disableAuth=true, datadog enabled, in-cluster postgresql). Co-authored-by: openhands <openhands@all-hands.dev>
The Replicated lint job (.github/workflows/lint-replicated-release.yml, Makefile target `build/openhands-0.7.15.tgz`) rewrites every umbrella dependency that has a sibling directory under `charts/` to use a `file://../<dep>` repository before running `helm package -u`. For that rewrite to resolve, the umbrella's `version:` constraint must match the sibling's local `Chart.yaml` version. With charts/integrations-hub/Chart.yaml version: 0.1.0 charts/openhands/Chart.yaml integrations-hub: 0.1.0-alpha.614 helm fails with "can't get a valid version for repositories integrations-hub" because the local sibling has no pre-release version matching the alpha constraint -- which is exactly the failure on run 25837636609 (#614). Point the umbrella's integrations-hub dep at 0.1.0 to match the local sibling, mirroring how runtime-api/plugin-directory/crd-check are wired today. The preview-helm-charts workflow already calls yq -i '(.dependencies[] | select(.name == "integrations-hub")).version = "${INTEGRATIONS_HUB_PREVIEW}"' charts/openhands/Chart.yaml before publishing, so the OCI-published umbrella will still carry the alpha pin (0.1.0-alpha.<PR_NUMBER>) when consumers pull it. Verified by running the Makefile recipe locally (yq dep rewrite + helm package -u): "Successfully packaged chart and saved it to build/openhands-0.7.15.tgz". Co-authored-by: openhands <openhands@all-hands.dev>
…ations-hub)
Background
----------
The integrations-hub image is a single FastAPI container that serves
both its Next.js SPA bundle and the /api/* routes. On the
`development` branch we now bake the SPA with
`NEXT_PUBLIC_BASE_PATH=/integrations-hub` and ship
`INTHUB_ROOT_PATH=/integrations-hub` as the runtime default, so the
image is ready to live behind `<oh_host>/integrations-hub/...` without
any ingress-level rewrite.
This change exposes that path as a single chart value -- `rootPath` --
and threads it through every place that needed to agree on it.
Changes
-------
charts/integrations-hub/values.yaml
* New top-level `rootPath: "/integrations-hub"` field, with a long
comment explaining what it controls (subchart probes, INTHUB_ROOT_PATH
env var, parent ingress, image NEXT_PUBLIC_BASE_PATH).
* Setting it to "" deliberately clears the image default so the hub
serves from the cluster root.
charts/integrations-hub/templates/_env.yaml
* Always render `INTHUB_ROOT_PATH` from `.Values.rootPath` (including
the empty-string case, so an override actually wins over the image
default).
* Documented the variable in the file header next to the other
INTHUB_* env vars.
charts/integrations-hub/templates/deployment.yaml
* Probe paths are now prefixed by `.Values.rootPath` so they hit
`/integrations-hub/api/health` by default and `/api/health` when
rootPath is empty -- matching whatever path the backend just
mounted. Defining `$healthPath` once keeps the three probe
declarations from drifting apart.
charts/openhands/templates/ingress-integrations-hub.yaml
* Drop the stale `/api/integrations-hub` rule -- the backend never
serves anything there; its API routes live under
`/integrations-hub/api/...` thanks to INTHUB_ROOT_PATH. Keeping the
second rule routed to the same service was harmless in practice
but confusing.
* The remaining rule's path is sourced from
`(index .Values "integrations-hub" "rootPath")` (defaulting to
`/integrations-hub`) so the ingress, the container env, and the
SPA bundle stay in lockstep.
* Added a block comment up top explaining why we deliberately do NOT
set an ingress-level `rewrite-target` annotation.
charts/openhands/values.yaml
* Mirrored the new `rootPath` field into the parent values so cloud
deployments can override it just like other integrations-hub knobs,
with a comment pointing at all three places it has to agree.
Validation
----------
* `helm lint charts/integrations-hub` -> 0 failures (only the
pre-existing "icon is recommended" note).
* `helm template intg charts/integrations-hub` with the default
rootPath: `INTHUB_ROOT_PATH=/integrations-hub` and all three probe
paths render as `/integrations-hub/api/health`.
* Same template with `--set rootPath=""`:
`INTHUB_ROOT_PATH=""` and probes fall back to plain
`/api/health`, confirming the empty-override path works end-to-end.
* `helm template oh charts/openhands -s
templates/ingress-integrations-hub.yaml` (run against a locally-built
copy of the subchart, since the OCI 0.1.0 release isn't published
yet): renders a single Ingress with `path: "/integrations-hub"` by
default and `path: "/foo"` when overridden via
`--set integrations-hub.rootPath=/foo`.
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid Helm chart structure following best practices, but needs security fixes and checklist completion before merge.
| admin: | ||
| # Comma-separated list of admin email addresses. |
There was a problem hiding this comment.
🟠 Important - Default Values: Hardcoded production email addresses in chart defaults.
Default values should use placeholders or be empty. Real email addresses create confusion about whether these are required values or just examples.
| admin: | |
| # Comma-separated list of admin email addresses. | |
| # Comma-separated list of admin email addresses. | |
| emails: "" |
Add documentation in the (missing) README.md showing an example configuration.
| {{- if .Values.ingress.root.annotations }} | ||
| {{ .Values.ingress.root.annotations | toYaml | nindent 4 }} | ||
| {{- else }} | ||
| {{ .Values.ingress.annotations | toYaml | nindent 4 }} | ||
| {{- end }} | ||
| spec: |
There was a problem hiding this comment.
🟡 Suggestion - Documentation: Clarify when ingress.root.annotations differs from ingress.annotations.
This fallback pattern exists but isn't documented. Add a brief comment explaining when root.annotations would be set differently (e.g., different ingress classes for root vs subpaths, different auth requirements).
If they're always the same, simplify to just use .Values.ingress.annotations.
| @@ -0,0 +1,11 @@ | |||
| apiVersion: v2 | |||
There was a problem hiding this comment.
🟠 Important - Missing Documentation: No README.md for the new chart.
Helm charts should include a README.md with:
- Overview of what the chart deploys
- Prerequisites (PostgreSQL setup, required secrets)
- Installation instructions
- Configuration examples (GCP Cloud SQL vs in-cluster PostgreSQL)
- Values documentation (or auto-generated from values.yaml comments)
- Upgrade notes
This is especially important for a new chart where users need guidance on the GCP vs non-GCP configurations, secret creation, and the rootPath / NEXT_PUBLIC_BASE_PATH coordination.
Review Summary[CRITICAL ISSUES] (Must fix before merge)
[IMPROVEMENT OPPORTUNITIES] (Should fix)
[TESTING GAPS]
[RISK ASSESSMENT]
Factors increasing risk:
Factors decreasing risk:
Recommendation: Address the critical password security issue before merge. Complete the PR checklist items (especially testing upgrade path and adding README.md). The optional nature of the dependency limits blast radius, but the chart should be production-ready before initial release. VERDICT: KEY INSIGHT:
|
Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Solid Helm chart implementation following best practices. The main issues are documentation gaps and configuration defaults that should be addressed before merge.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solid Helm chart structure following best practices. The init container database setup logic could be simplified, and the PR checklist needs completion before merge.
[IMPROVEMENT OPPORTUNITIES]
- [deployment.yaml, Init Containers] Complexity: Inline SQL script with heredocs is error-prone; consider ConfigMap approach (see inline comment)
- [deployment.yaml, Line 103] Consistency: Two different wait strategies for init containers (see inline comment)
- [PR Description] Incomplete Checklist: The Helm Chart Checklist is not completed - all boxes should be checked or marked N/A before merge
[UNRESOLVED FROM PREVIOUS REVIEWS]
These issues were flagged in previous reviews and still need attention:
- Missing README.md for the chart (Important)
- Legacy PostgreSQL image with global security bypass flag (Important)
- Hardcoded production email addresses in chart defaults (Important)
- Missing documentation for deduplication implementation and annotation fallback patterns (Suggestion)
[RISK ASSESSMENT]
This PR adds a new optional infrastructure component (integrations-hub service + PostgreSQL dependency) with isolated blast radius. Medium risk due to:
- New infrastructure dependency (PostgreSQL database)
- Complex init container logic for database setup
- Unresolved security/documentation issues from previous reviews
- Incomplete PR checklist
Mitigating factors: Optional dependency (disabled by default), follows existing Helm chart patterns, no changes to core OpenHands functionality.
Recommendation: Address the incomplete PR checklist and consider the improvement suggestions before merge. The unresolved issues from previous reviews should also be tracked/addressed.
VERDICT:
✅ Worth merging after checklist completion - Core chart structure is sound, follows best practices
KEY INSIGHT:
The chart architecture is solid, but init container database setup complexity could cause operational issues in edge cases - consider externalizing SQL scripts to ConfigMaps for better maintainability.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.
| if psql -h "$DB_HOST" -p "$DB_PORT" -U postgres -d postgres -c "SELECT 1;" 2>&1; then | ||
| echo "PostgreSQL is up!" | ||
|
|
||
| echo "Creating the database $DB_NAME if it doesn't exist..." |
There was a problem hiding this comment.
🟡 Suggestion - Complexity: The inline SQL script with heredocs and psql meta-commands is complex and error-prone.
Consider:
- Moving the SQL to a ConfigMap for easier testing and maintenance
- Adding clearer error messages that identify which step failed
- Using a more consistent wait strategy (the
wait-for-postgrescontainer below uses a different pattern)
Example ConfigMap approach:
---
apiVersion: v1
kind: ConfigMap
metadata:
name: integrations-hub-init-sql
data:
init.sql: |
-- SQL commands hereThen mount and execute: psql ... -f /scripts/init.sql
| - name: wait-for-postgres | ||
| image: bitnamilegacy/postgresql:latest | ||
| command: ['sh', '-c'] | ||
| args: |
There was a problem hiding this comment.
🟡 Suggestion - Consistency: This init container uses a different wait strategy than create-db-user above (line 54):
create-db-user: 60 retries with 5 second sleep (5 minute timeout)wait-for-postgres: Infiniteuntilloop with 2 second sleep (no timeout)
Consider using a consistent pattern for both. The create-db-user approach with explicit timeout is safer.
Regenerates the lockfile via 'helm dependency update' to match the dependency declarations on main: - automation: 0.1.8 -> 0.1.9 (Chart.yaml has been at 0.1.9 since #634 etc.) - adds integrations-hub: 0.1.0 (declared in Chart.yaml in #614, never reflected in the lockfile) - digest + generated timestamp refreshed The lockfile had been drifting because earlier dependency-bump PRs only touched Chart.yaml. Same class of drift as #630. No functional behavior change in this PR — it brings the recorded lockfile into agreement with what helm dep update would produce anyway at build time. Laminar dependency unchanged at 0.1.11; the upgrade to 0.1.12 is tracked separately in #638.
* PLTF-2687: sync Chart.lock with current Chart.yaml dependencies Regenerates the lockfile via 'helm dependency update' to match the dependency declarations on main: - automation: 0.1.8 -> 0.1.9 (Chart.yaml has been at 0.1.9 since #634 etc.) - adds integrations-hub: 0.1.0 (declared in Chart.yaml in #614, never reflected in the lockfile) - digest + generated timestamp refreshed The lockfile had been drifting because earlier dependency-bump PRs only touched Chart.yaml. Same class of drift as #630. No functional behavior change in this PR — it brings the recorded lockfile into agreement with what helm dep update would produce anyway at build time. Laminar dependency unchanged at 0.1.11; the upgrade to 0.1.12 is tracked separately in #638. * Bump openhands chart version to 0.7.18 for Chart.lock sync Co-authored-by: openhands <openhands@all-hands.dev> --------- Co-authored-by: openhands <openhands@all-hands.dev>
4 participants
Description
Helm Chart Checklist
versionfield inChart.yamlfor each modified chartAdditional Notes