Review: Provision infrastructure in pal-e-services
Verdict: NEEDS_REFINEMENT
Template Completeness
- [x] Type — Feature
- [x] Lineage — Standalone, scoped from design spec
- [x] Repo — forgejo_admin/pal-e-services
- [x] User Story — Marcus as admin, infra provisioning
- [x] Context — sufficient background
- [x] File Targets — present with should/should-not-touch
- [x] Acceptance Criteria — 4 criteria listed
- [x] Test Expectations — tofu validate + plan command
- [x] Constraints — present
- [x] Checklist — present
- [x] Related — present
Traceability
- [x] story:read-ops — Admin Read Operations via GroupMe
- [x] story:write-ops — Admin Write Operations via GroupMe
- [x] arch:A4 — Deployment architecture component
- [x] Forgejo issue — forgejo_admin/westside-ai-assistant#1, open
File Targets
- [x]
terraform/k3s.tfvars— verified: file exists, contains bothservicesmap (line 104) andkeycloak_clientsmap (line 40). basketball-api reference entry confirmed at line 113. westside-app client reference confirmed at line 41. - [ ]
terraform/variables.tf— ISSUE: not listed as a file target, but MUST be modified. Thekeycloak_clientsvariable type (line 102-125) has noservice_accounts_enabledattribute. Adding a service-account client requires adding this attribute to the variable type object. - [ ]
terraform/keycloak.tf— ISSUE: not listed as a file target, but MUST be modified. Thekeycloak_openid_clientresource (line 91-118) does not setservice_accounts_enabled. Additionally, there is nokeycloak_openid_client_service_account_roleresource anywhere in the codebase — this is needed to map the admin role to the service account.
Repo Placement
Correct. Issue is filed on westside-ai-assistant but specifies work in forgejo_admin/pal-e-services, which is the correct repo for infrastructure provisioning. Single-repo change.
Dependencies
- [ ] Issue #2 (Create kustomize overlay in pal-e-deployments) — board item #594, backlog. The services entry's
source_path = "overlays/westside-ai-assistant/prod"does not exist yet in pal-e-deployments. ArgoCD will fail to sync until the overlay is created. Merge order: #1 first, #2 second, apply after both merged. Not documented in issue scope. - [x] Issue #3 (NetworkPolicy update) — board item #595, backlog. Post-deploy dependency, not blocking for this ticket.
Acceptance Criteria
- [ ] AC1: services entry — testable via tofu plan, but missing required fields. The AC says
port: 8000, funnel: true, source_repo, source_pathbut omitsforgejo_repoandimage_repo, which are REQUIRED fields per the variable definition in variables.tf (lines 130-131). - [ ] AC2: "Keycloak client westside-ai-bot is confidential, service account enabled" — NOT achievable with current variable type. The keycloak_clients variable has no
service_accounts_enabledattribute. The keycloak_openid_client resource does not set it. Variable + resource must both be extended. - [ ] AC3: "Client has realm role admin mapped" — NOT achievable with current codebase. No
keycloak_openid_client_service_account_roleresource exists. A new resource block must be added to keycloak.tf, plus a new variable attribute to drive it (e.g.,service_account_realm_roles). - [x] AC4: tofu plan shows expected resources — testable, but will only show correct resources once AC2/AC3 infrastructure is added.
Blast Radius
Low for the services entry — existing for_each pattern handles new entries automatically. Medium for the keycloak changes — modifying the variable type and resource affects ALL existing keycloak clients. The new attributes must use
optional() with safe defaults (false / []) to avoid breaking existing client entries. No other service currently uses service accounts, so this is a net-new capability addition to the platform. Rollback is straightforward: revert the tfvars entry.Decomposition Assessment
The ticket touches 3 files (
k3s.tfvars, variables.tf, keycloak.tf) in 1 repo with 4 AC. Once the file targets and AC are corrected, this is still achievable in a single agent pass (<5 min). The three files are tightly coupled (variable definition, resource usage, variable values) and cannot be parallelized. No decomposition needed.Recommendation
[BODY]Add file target:terraform/variables.tf— addservice_accounts_enabled = optional(bool, false)andservice_account_realm_roles = optional(list(string), [])to the keycloak_clients variable type object.[BODY]Add file target:terraform/keycloak.tf— addservice_accounts_enabled = each.value.service_accounts_enabledto the keycloak_openid_client resource block, and add a newkeycloak_openid_client_service_account_realm_roleresource for service account role mapping.[BODY]Fix AC1: addforgejo_repo: "forgejo_admin/westside-ai-assistant"andimage_repo: "westside-ai-assistant/api"to the expected services entry fields.[BODY]Add dependency note to Constraints section: "Depends on issue #2 (kustomize overlay) before tofu apply. Merge order: #1 first, #2 second, apply after both merged."[BODY]Clarify in File Targets "should NOT touch" section: remove any implication that variables.tf and keycloak.tf are off-limits — they must be modified to support service accounts.