Skip to content

fix(apps): make actual-budget openid schema offline-resolvable - #2507

Merged
devantler merged 1 commit into
mainfrom
claude/actualbudget-offline-schema
Jul 6, 2026
Merged

devantler merged 1 commit into
mainfrom
claude/actualbudget-offline-schema

Conversation

@devantler

Copy link
Copy Markdown
Contributor

🤖 Generated by the Daily AI Assistant

Why

Actual-budget's OpenID config satisfied the chart's values-schema through an on-cluster-only Secret injection, so any offline validation (ksail's in-process Helm render, being re-enabled in #2273) couldn't resolve it and silently skipped validating actual-budget's rendered manifests. CI was never actually checking this app.

What

Switches the schema-satisfaction to the statically-resolvable existingSecret branch so offline render and live upgrade both validate. The running app is unchanged — verified render is byte-identical (the chart gates this config behind an ingress we don't use; the live OIDC config stays the injected env), and OIDC login is unaffected.

Prod-values change, so promote deliberately. Unblocks proper offline validation once #2273 lands.

Fixes #2385

Switch the chart's login.openid oneOf satisfaction from a valuesFrom
clientSecret injection (on-cluster-only) to the existingSecret branch,
which resolves statically. Offline render (ksail in-process Helm
validate) and live upgrade now both match exactly one oneOf branch.
Removing valuesFrom is mandatory: with a clientSecret and existingSecret
both present, two branches match and "exactly one" fails.

Render is byte-identical (the chart gates the openid env/secret on
ingress.enabled, disabled here), so the running app is unchanged; the
live OIDC config remains the postRenderer-injected ACTUAL_OPENID_* env.

Fixes #2385

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c1dcedd0-2cff-4f80-a638-38e9f3600865

📥 Commits

Reviewing files that changed from the base of the PR and between 4ee654a and 7eb28ee.

📒 Files selected for processing (1)
  • k8s/bases/apps/actual-budget/helm-release.yaml
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • devantler-tech/actions (auto-detected)
  • devantler-tech/aws (auto-detected)
  • devantler-tech/ksail (auto-detected)
  • devantler-tech/reusable-workflows (auto-detected)
  • devantler-tech/ascoachingogvaner (auto-detected)
  • devantler-tech/unifi (auto-detected)
  • devantler-tech/wedding-app (auto-detected)
  • devantler-tech/provider-upjet-unifi (auto-detected)
  • devantler-tech/agent-skills (auto-detected)
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (2)
k8s/**

📄 CodeRabbit inference engine (AGENTS.md)

Use Kustomize overlays under k8s/: keep k8s/bases/ immutable from overlays and make changes via patches: in provider/cluster overlays only.

Files:

  • k8s/bases/apps/actual-budget/helm-release.yaml
k8s/**/*.{yaml,yml}

📄 CodeRabbit inference engine (AGENTS.md)

Never commit plaintext secrets; all secrets must be SOPS-encrypted and use the *.enc.yaml suffix.

Files:

  • k8s/bases/apps/actual-budget/helm-release.yaml
🧠 Learnings (1)
📚 Learning: 2026-07-01T21:13:36.950Z
Learnt from: devantler
Repo: devantler-tech/platform PR: 2359
File: k8s/bases/apps/actual-budget/helm-release.yaml:62-111
Timestamp: 2026-07-01T21:13:36.950Z
Learning: When reviewing Kustomize/Helm YAML in this repo, keep the base vs provider overlay split: `k8s/bases/apps/**` and `k8s/bases/infrastructure/**` should contain each app’s full, environment-agnostic configuration (including base-level postRenderer Kustomize patches such as deployment strategy, topology spread, probes, and env injection). `k8s/providers/{docker,hetzner}/**` should only add small provider-specific deltas (e.g., `interval`, `persistence.size`) via patch files (like `k8s/providers/<provider>/apps/<app>/patches/helm-release-patch.yaml`). If configuration is identical across providers (e.g., OIDC/OAuth env vars where `${domain}` is resolved per cluster via envsubst), it belongs in the base and must not be duplicated into provider overlays.

Applied to files:

  • k8s/bases/apps/actual-budget/helm-release.yaml
🪛 Trivy (0.69.3)
k8s/bases/apps/actual-budget/helm-release.yaml

[info] 8-227: limit range usage

A LimitRange policy with a default requests and limits for each container should be configured

Rule: KSV-0039

Learn more

(IaC/Kubernetes)

🔀 Multi-repo context devantler-tech/ksail, devantler-tech/ascoachingogvaner

Linked repositories findings

devantler-tech/ksail

  • pkg/svc/gitops/render/resolver.go:70, 276, 304, 347 and pkg/svc/gitops/render/render.go:299, 332 show valuesFrom is only resolved from in-repo ConfigMaps/Secrets during offline render. This supports the PR’s switch away from valuesFrom for chart-schema satisfaction, since that path is not offline-resolvable. [::devantler-tech/ksail::]
  • pkg/cli/cmd/workload/render.go:148 emits the skipped Helm render for HelmRelease ... warning, and pkg/cli/cmd/workload/validate_render_test.go:151, 238 assert the warning behavior. This is the validation path the issue is trying to avoid for actual-budget. [::devantler-tech/ksail::]
  • charts/ksail-operator/templates/oidc-secret.yaml:14-17, charts/ksail-operator/templates/_helpers.tpl:52-53, and charts/ksail-operator/values.yaml:80-85 use an existingSecret pattern for OIDC secrets in the operator chart itself. That’s a nearby precedent, but not a direct dependency on actual-budget. [::devantler-tech/ksail::]

devantler-tech/ascoachingogvaner

  • No matches for actual-budget, login.openid, existingSecret, clientSecretKey, ACTUAL_OPENID, or valuesFrom were found. No cross-repo consumer of this change was observed here. [::devantler-tech/ascoachingogvaner::]

Other repositories

  • The broad search returned no relevant matches in devantler-tech/aws, devantler-tech/reusable-workflows, devantler-tech/wedding-app, devantler-tech/provider-upjet-unifi, or devantler-tech/agent-skills for the OpenID/actual-budget schema path.
🔇 Additional comments (1)
k8s/bases/apps/actual-budget/helm-release.yaml (1)

207-215: LGTM!

Also applies to: 225-227


📝 Walkthrough

Walkthrough

Changes

The Actual Budget HelmRelease switches OIDC client-secret handling from valuesFrom injection to a postRenderers Kustomize patch that edits the rendered Deployment, adding a startup probe, topology spread constraints, and ACTUAL_OPENID_* environment variables. The login.openid values now use existingSecret with actual-budget-oidc and client-secret, and the surrounding comments were updated to describe the schema oneOf branch selection.

Sequence Diagram(s)

Included above within the hidden review stack artifact.

Estimated code review effort: High

Related issues: #2385

Related PRs: None noted

Suggested labels: apps, actual-budget, helm, kubernetes

Suggested reviewers: devantler

🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: making actual-budget's OpenID schema satisfaction offline-resolvable.
Description check ✅ Passed The description is directly related to the same offline-resolvable OpenID schema fix and its rollout impact.
Linked Issues check ✅ Passed The change aligns with #2385 by switching to existingSecret and removing valuesFrom, which is the required offline-resolvable schema path.
Out of Scope Changes check ✅ Passed No clear out-of-scope code changes are evident beyond the requested HelmRelease schema/value adjustment.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

Warning

Review ran into problems

🔥 Problems

Linked repositories: Couldn't analyze devantler-tech/unifi - clone failed: 2 UNKNOWN:

<title>500 Server Error</title>

Error: Server Error

The server encountered an error and could not complete your request.

Please try again in 30 seconds.


Comment @coderabbitai help to get the list of available commands.

@devantler

Copy link
Copy Markdown
Contributor Author

You did not check the failed Pre-merge checks from coderabbitai: #2507 (comment)

Remember to check these, as otherwise you might make draft PRs end up stranded, as I am not going to promote them, when pre-merge checks ar note green.

@devantler

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Re the Out of Scope Changes pre-merge check (inconclusive): this PR is genuinely schema-only. The diff (+12 -8, single file k8s/bases/apps/actual-budget/helm-release.yaml) does exactly three things — removes the login.openid.clientSecret valuesFrom injection, adds the login.openid.existingSecret branch, and updates the adjacent comment. The startupProbe, topologySpreadConstraints, and env-injection lines are pre-existing postRenderers patches in the same file, shown only as unchanged diff context — they are not modified here. Please re-run the pre-merge checks against the actual hunks.

🤖 Generated by the Daily AI Assistant

@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr diff 2507 --patch

Length of output: 2005


🏁 Script executed:

#!/bin/bash
curl -sL https://github.com/devantler-tech/platform/pull/2507.diff

Length of output: 2116


@devantler, confirmed — I pulled the actual diff and it matches exactly what you described:

  • Removed: the valuesFrom block (5 lines) that injected login.openid.clientSecret from the actual-budget-oidc Secret.
  • Updated: the adjacent comment block explaining the oneOf schema-satisfaction rationale.
  • Added: the login.openid.existingSecret branch (name: actual-budget-oidc, clientSecretKey: client-secret).

The postRenderers block (including startupProbe, topologySpreadConstraints, and the ACTUAL_OPENID_* env injection) appears in the diff only as unchanged surrounding context — it's not modified by this PR. So the "Out of Scope Changes" check should indeed have been conclusively in-scope; I'll flag this for re-evaluation and re-run the checks now.


✏️ Learnings added
Learnt from: devantler
URL: https://github.com/devantler-tech/platform/pull/2507

Timestamp: 2026-07-06T16:23:21.598Z
Learning: In devantler-tech/platform, for k8s/bases/apps/actual-budget/helm-release.yaml, the `postRenderers` Kustomize patch block (including `startupProbe`, `topologySpreadConstraints`, and `ACTUAL_OPENID_*` env injection) is pre-existing config unrelated to changes that only touch `login.openid` schema satisfaction (switching between `valuesFrom` and `existingSecret` branches). When such lines appear in a diff hunk purely as unchanged context near an actual edit, they should not be flagged as out-of-scope changes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@devantler
devantler marked this pull request as ready for review July 6, 2026 16:29
@devantler

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

The Out of Scope Changes pre-merge check is inconclusive because the walkthrough summary references "extra deployment patching (startupProbe, topologySpreadConstraints, env injection)". Those are not part of this PR's diff — please regenerate the walkthrough.

The complete diff is a single file, k8s/bases/apps/actual-budget/helm-release.yaml, +12 / -8, three hunks only:

  1. Remove the spec.valuesFrom block (the login.openid.clientSecret injection).
  2. Expand the explanatory comment above login:.
  3. Add login.openid.existingSecret (name + clientSecretKey).

The postRenderers Kustomization patches (startupProbe / topologySpreadConstraints / env) are pre-existing, unchanged lines shown only as diff context — no hunk adds, removes, or edits them. This PR moves the OpenID schema from an on-cluster-only valuesFrom clientSecret to the static existingSecret branch so ksail's offline Helm validate resolves the oneOf. Nothing outside that schema change is touched.

🤖 Generated by the Daily AI Assistant

@coderabbitai

coderabbitai Bot commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor

@devantler Understood — the walkthrough's "extra deployment patching (startupProbe, topologySpreadConstraints, env injection)" phrasing is misleading since those lines are pre-existing postRenderers config shown only as diff context, not modified in this PR. The actual diff remains a single-file, +12/-8 change limited to three hunks: removing the valuesFrom block, expanding the explanatory comment, and adding the login.openid.existingSecret branch.

I'll trigger a full re-review so the walkthrough and pre-merge checks are regenerated against the actual scoped diff.


✏️ Learnings added
Learnt from: devantler
URL: https://github.com/devantler-tech/platform/pull/2507

Timestamp: 2026-07-06T17:18:03.214Z
Learning: In devantler-tech/platform, for k8s/bases/apps/actual-budget/helm-release.yaml, PR `#2507`'s AI-generated walkthrough summary incorrectly attributed the pre-existing `postRenderers` Kustomize patch (startupProbe, topologySpreadConstraints, ACTUAL_OPENID_* env injection) as part of the changeset. The actual diff only touches the `login.openid` schema-satisfaction lines: removing `spec.valuesFrom` (clientSecret injection), expanding the explanatory comment, and adding the `login.openid.existingSecret` branch (name + clientSecretKey). Walkthrough generation for this file should not describe the postRenderers block as modified when it appears only as unchanged diff context.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Full review finished.

@devantler
devantler added this pull request to the merge queue Jul 6, 2026
Merged via the queue into main with commit 3f49bc1 Jul 6, 2026
16 checks passed
@devantler
devantler deleted the claude/actualbudget-offline-schema branch July 6, 2026 17:58
@github-project-automation github-project-automation Bot moved this from 🫴 Ready to ✅ Done in 🌊 Project Board Jul 6, 2026
@botantler-1

botantler-1 Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.106.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

fix(apps): make actual-budget chart-schema satisfaction offline-resolvable (existingSecret instead of valuesFrom clientSecret)

1 participant