Repository navigation
Replace Engine with Worker - #34
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Helm chart to replace the “engine” component with a “worker” component, including renaming values keys and updating templates to deploy and wire the worker service.
Changes:
- Rename values/config from
enginetoworker(including production/CI overlays and schema). - Update Kubernetes manifests (Deployment/Service/HPA/PDB/ServiceMonitor/NetworkPolicy/ServiceAccount) to target the worker component and port
3000. - Replace
OPS_ENGINE_URLwithOPS_WORKER_URLand addOPS_CONTAINER_TYPEenv var(s).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| chart/values.yaml | Renames engine config/env to worker and updates ingress comment reference. |
| chart/values.schema.json | Renames schema section from engine to worker and updates description text in that section. |
| chart/values.production.yaml | Updates production overlay key from engine to worker. |
| chart/values.ci.yaml | Updates CI overlay key from engine to worker. |
| chart/templates/servicemonitor.yaml | Renames the engine ServiceMonitor to worker and points to worker metrics path. |
| chart/templates/serviceaccount-worker.yaml | Switches ServiceAccount creation/labels from engine to worker. |
| chart/templates/service-worker.yaml | Renames service to worker and changes service port/targetPort to 3000. |
| chart/templates/secret-env.yaml | Updates secret auto-generation env sources to include .Values.worker.env instead of engine. |
| chart/templates/pdb-worker.yaml | Switches PDB values/labels from engine to worker. |
| chart/templates/networkpolicy.yaml | Updates network policies to reference worker component and port 3000. |
| chart/templates/hpa-worker.yaml | Switches HPA target/values from engine to worker. |
| chart/templates/external-secret.yaml | Updates external secret env aggregation from engine to worker. |
| chart/templates/deployment-worker.yaml | Renames deployment/image/env wiring from engine to worker; updates container port to 3000 and adds OPS_CONTAINER_TYPE=WORKER. |
| chart/templates/deployment-app.yaml | Adds OPS_CONTAINER_TYPE=APP to the app deployment. |
| chart/templates/_helpers.tpl | Replaces openops.engineServiceUrl with openops.workerServiceUrl and updates port to 3000. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Every PDB in the chart set minAvailable, while analytics and tables default to a single replica. minAvailable: 1 against one replica evaluates to disruptionsAllowed: 0, so those pods cannot be evicted and every voluntary disruption fails: cluster upgrades, node image upgrades and autoscaler scale-down all stall while it is set. This is not theoretical. It left an Azure AKS estate on three-month-old node images across four clusters, with the pools reporting Failed and nothing alerting on it, because the drain could never complete. maxUnavailable: 1 is equivalent at two replicas and drainable at one, so all five components now default to it rather than only fixing the two that deadlock today. Overriding replicas down to 1 is a supported thing to do and should not reintroduce the deadlock. minAvailable still works when maxUnavailable is unset, so existing overrides are unaffected. A PDB may not set both; maxUnavailable takes precedence. One behaviour change to note: at three or more replicas maxUnavailable: 1 permits one pod down at a time where minAvailable: 1 permitted all but one. That is safer but makes drains slower. tables uses ReadWriteOnce storage and cannot be scaled past one replica, so it still incurs brief downtime while its node drains. This makes the drain possible, not seamless. Verified by rendering the chart: all five PDBs emit maxUnavailable: 1 with default values, and an override of maxUnavailable: null with minAvailable: 2 still emits minAvailable: 2. helm lint and both CI template steps pass. Note that CI never exercises this path, since values.ci.yaml disables PDBs. Part of OPS-4725
The template guarded on truthiness, and Go templates treat 0 as false, so maxUnavailable: 0 fell through to rendering minAvailable: 1 — silently producing a PDB that differs from the values that asked for it. 0 is a valid PodDisruptionBudget value. kindIs "invalid" tests for nil instead, which keeps all four cases correct: an explicit 0 renders as 0, an explicit null falls back to minAvailable, an absent key falls back, and a set value renders. hasKey would not work here, since it is true for maxUnavailable: null and would render an empty field. Also correct two documentation errors. The README claimed a PDB "may not set both" fields and then that maxUnavailable "wins if you set both", conflating the rendered resource with the values schema. AGENTS.md described PDBs as covering "all stateless components" while tables, which has one, is stateful. Part of OPS-4725
e2e is red for a reason that predates this PR: no released openops-worker image exists in any public registry, and the last released openops-app (0.6.25) predates the engine→worker rename this branch carries, so it crash-loops on OPS_ENGINE_URL. It clears with the 0.6.26 release.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical release-secret mismatch and moderate chart, compatibility, registry, and documentation issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
AGENTS.md:45
- The documented secret name does not match the workflow: release.yml passes
secrets.AZURE_ACR_RELEASE_CLIENT_IDto azure/login, while this line tells operators to configureAZURE_ACR_CLIENT_ID. Following this documentation leaves OIDC login without the client ID and the release fails; please use the workflow's actual secret name here.
- Authenticates with GitHub OIDC. Required secrets: `AZURE_ACR_CLIENT_ID`, `AZURE_TENANT_ID`, `AZURE_SUBSCRIPTION_ID`; required vars: `ACR_PUBLIC_NAME`, `ACR_PUBLIC_LOGIN_SERVER`.
chart/values.yaml:220
- Renaming the values key from
enginetoworkermakes existing override files silently lose their engine replicas, resources, service-account settings, and other overrides on upgrade; the schema does not reject the now-unknownenginekey. Preserve a compatibility alias or add an explicit migration/validation path before replacing the public values API.
worker:
name: openops-worker
image: openops-worker
repository: "" # Override global image.repository (empty = use global)
chart/values.yaml:259
- Because
openops.imageRepositoryprefers a non-empty component repository, this value prevents the customimage.repositorymirror documented in README.md from applying to tables. A deployment using only its own private mirror will still pullopenops.azurecr.io/openops-tables. Leave this override empty (the default still resolves to the public ACR) or document the required component-specific override.
repository: "openops.azurecr.io" # Always the public registry for tables
chart/values.yaml:322
- The same non-empty component override also makes the documented custom
image.repositoryineffective for analytics, so a private mirror deployment still pullsopenops.azurecr.io/openops-analytics. Leave this override empty so it follows the global repository, or explicitly document the additional override required for analytics.
repository: "openops.azurecr.io" # Always the public registry for analytics
- Files reviewed: 24/24 changed files
- Comments generated: 8
- Review effort level: Lite
| aws-access-key-id: ${{ secrets.ECR_ACCESS_KEY_ID }} | ||
| aws-secret-access-key: ${{ secrets.ECR_SECRET_ACCESS_KEY }} | ||
| aws-region: ${{ vars.ECR_PUBLIC_REGION }} | ||
| client-id: ${{ secrets.AZURE_ACR_RELEASE_CLIENT_ID }} |
Comment on lines
+331
to
+332
| image: | ||
| repository: my-registry.example.com/openops |
| } | ||
| }, | ||
| "engine": { | ||
| "worker": { |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate security, deployment, networking, secret-rotation, and release-workflow issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
.github/workflows/release.yml:65
- The documented release secret is
AZURE_ACR_CLIENT_ID(AGENTS.md:47), but this step readsAZURE_ACR_RELEASE_CLIENT_ID. With the documented environment secret configured, Azure login receives an empty client ID and the publish job fails. Use the same secret name in the workflow and documentation.
client-id: ${{ secrets.AZURE_ACR_RELEASE_CLIENT_ID }}
chart/templates/deployment-mcp.yaml:39
- When MCP uses a pre-created service account (
mcp.serviceAccount.create: false), this helper call resolves to the literaldefaultservice account and ignoresmcp.serviceAccount.name; the helper only honors the configured name whencreateis true. That prevents the documented/custom IRSA or Workload Identity account from being used. Make the service-account-name helper honor the configured name in both create modes.
serviceAccountName: {{ include "openops.serviceAccountName" (dict "root" . "component" "mcp") }}
chart/templates/secret-env.yaml:15
- Appending
mcp.envadds OAuth credentials to the generated Secret, butopenops.secretChecksumonly hashes explicitsecretEnv.data/stringData. ChangingOPS_OAUTH_RS_CLIENT_SECRETtherefore updates the Secret without changing the MCP pod template, so running pods retain the old value and OAuth token exchange fails until they are manually restarted. Include the generated environment data in the rollout checksum or use a secret-reload mechanism.
{{- $envSources = append $envSources .Values.mcp.env -}}
chart/values.yaml:158
- This changes the default runner image from a concrete image to an empty string. If an operator enables the existing
OPS_SUBAGENTS_ENABLED=trueKubernetes executor without another override, the app will create subagent jobs with no image and execution will fail. Preserve a valid image in the new registry or add render-time validation/documentation that an image override is mandatory when subagents are enabled.
OPS_SUBAGENT_RUNNER_IMAGE: ""
chart/values.yaml:317
- When
secretEnv.existingSecretis used without the chart's generated ExternalSecret, this derived variable is rendered as asecretKeyRefforOPENOPS_MCP_CLIENT_SECRET, while the configuration only asks operators to provideOPS_OAUTH_RS_CLIENT_SECRET. No alias is created in an existing Secret, so the optional reference is missing and MCP OAuth exchanges fail. Either map the pod to the original key or explicitly require/document the derived key for externally managed Secrets.
OPENOPS_MCP_CLIENT_SECRET: '{{ .Values.openopsEnvSecrets.OPS_OAUTH_RS_CLIENT_SECRET }}'
docs/DEPLOY_TO_AWS_EKS.md:620
- This registry edit leaves the rest of the AWS production example using the removed
enginevalues (for example theserviceAccount.engineandengine:blocks later in the file). Those keys are now ignored by the chart, so users following this guide will not get the documented worker replica count or service-account configuration. Rename the remaining engine examples, labels, and commands toworker.
docs/DEPLOY_TO_AWS_EKS_FARGATE.md:680 - This registry edit leaves the Fargate guide's component example using the removed
enginevalues later in the file. Those keys are ignored by the chart, so the documented replica, service-account, and operational settings will not apply after the rename. Rename the remaining engine examples, labels, and commands toworker.
- Files reviewed: 32/32 changed files
- Comments generated: 4
- Review effort level: Lite
| {{- fail "ERROR: OPS_OAUTH_RS_CLIENT_SECRET must be at least 32 characters when mcp.enabled is true. Generate with: openssl rand -hex 32" -}} | ||
| {{- end -}} | ||
| {{- $url := include "openops.publicUrl" . -}} | ||
| {{- if and (not (hasPrefix "https://" $url)) (not (hasPrefix "http://localhost" $url)) -}} |
Comment on lines
+32
to
+34
| {{- with include "openops.secretChecksum" . }} | ||
| checksum/secret-env: {{ . }} | ||
| {{- end }} |
Comment on lines
+318
to
+324
| - to: | ||
| - podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/component: app | ||
| ports: | ||
| - protocol: TCP | ||
| port: 80 |
Comment on lines
+332
to
+336
| - to: | ||
| - namespaceSelector: {} | ||
| ports: | ||
| - protocol: TCP | ||
| port: 443 |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate issues affect release authentication, MCP security and networking, secret rotation, and worker configuration.
Review details
Suppressed comments (12)
Previously missed (2) — in code that hasn't changed since the last review.
README.md:539
- This replacement still documents a top-level
hpa.workermap, but the chart's HPA template reads.Values.worker.autoscalingand there is nohpavalue invalues.yaml. Copying this example therefore leaves worker autoscaling disabled; update the example to the actual values shape.
README.md:765 - The chart does not read a top-level
serviceAccountmap:serviceaccount-worker.yamluses.Values.worker.serviceAccount. Consequently this newly renamedworkerblock is ignored and the documented IAM role is never attached; nest it underworker.serviceAccount(and update the surrounding component examples consistently).
.github/workflows/release.yml:65
- AGENTS.md documents
AZURE_ACR_CLIENT_IDas the required release secret, but this workflow readsAZURE_ACR_RELEASE_CLIENT_ID. A release configured according to the documented secret name will pass an empty client ID toazure/loginand fail; align the workflow and documentation names.
client-id: ${{ secrets.AZURE_ACR_RELEASE_CLIENT_ID }}
README.md:22
- The worker rename is not carried through the deployment guides:
docs/DEPLOY_TO_AWS_EKS.mdstill tells users to configureengine/openops-engine, and the Fargate guide still containsenginevalues plus engine log and health selectors. Following those examples with this chart leaves the worker configuration ignored and diagnostics target nonexistent resources; update the guides as part of this rename.
- **openops-worker**: Workflow execution worker.
- **openops-mcp** (opt-in): MCP server that lets external agents such as Claude Code or Codex operate OpenOps over OAuth.
chart/templates/_helpers.tpl:440
- This prefix check accepts
http://localhost.evil.comandhttp://localhost@evil.comas the plain-HTTP exception. With MCP enabled, that permits an insecure OAuth issuer on a non-localhost host; require a hostname boundary, such as exacthttp://localhostorhttp://localhost:<port>.
{{- if and (not (hasPrefix "https://" $url)) (not (hasPrefix "http://localhost" $url)) -}}
chart/templates/configmap-nginx.yaml:83
- When HTTPS terminates at the ingress or load balancer, this Nginx server still listens on port 80, so
$schemeishttp. This overwrites the incomingX-Forwarded-Proto: httpsand makes MCP see requests as HTTP during OAuth flows; preserve the trusted forwarded header, with an HTTP fallback for local direct access, instead.
proxy_set_header X-Forwarded-Proto $scheme;
chart/templates/deployment-mcp.yaml:33
openops.secretChecksumonly hashessecretEnv.data/stringData, not the auto-generated entries fromopenopsEnvSecretsandmcp.env. RotatingOPS_OAUTH_RS_CLIENT_SECRETtherefore updates the Secret without changing this annotation, so existing app and MCP pods keep the old environment value and OAuth fails until they are manually restarted. Include generated secret data in the checksum or explicitly roll these deployments when the shared secret changes.
{{- include "openops.prometheusAnnotations" (dict "root" . "component" "mcp") | nindent 8 }}
{{- with include "openops.secretChecksum" . }}
checksum/secret-env: {{ . }}
chart/templates/networkpolicy.yaml:322
- With the default
networkPolicy.enabled: true, this egress rule is not sufficient for MCP startup: the app policy still permits ingress only from pods labelednginx, while the MCP pod callsopenops-appdirectly for its API/OpenAPI and OAuth exchange. Add an app-policy ingress rule allowing themcpcomponent on port 80, otherwise the enabled MCP deployment cannot reach the API.
- to:
- podSelector:
matchLabels:
app.kubernetes.io/component: app
ports:
chart/templates/networkpolicy.yaml:336
namespaceSelector: {}selects pods in Kubernetes namespaces; it does not allow egress to external IPs such as Logz.io. With NetworkPolicy enabled, the MCP pod'sLOGZIO_TOKENpath cannot reach an external HTTPS endpoint despite this rule and the README claiming port 443 is allowed. Add an appropriately scopedipBlockfor the required destinations or document that external shipping is unavailable under this policy.
- to:
- namespaceSelector: {}
ports:
- protocol: TCP
port: 443
chart/values.yaml:158
- Setting the default runner image to an empty string regresses the opt-in Kubernetes subagent path: when
OPS_SUBAGENTS_ENABLED=true, the app receives noOPS_SUBAGENT_RUNNER_IMAGE, and this chart provides no validation or documented required override. Keep a valid runner image in the new registry or fail rendering when subagents are enabled; otherwise enabling subagents produces unusable jobs.
OPS_SUBAGENT_RUNNER_IMAGE: ""
docs/DEPLOY_TO_AWS_EKS.md:620
- This registry update leaves the rest of the guide on the pre-rename
engineAPI (service account, replicas/resources, PDB/HPA, and log commands). Helm now readsworker, so those copiedengine:settings are ignored and the guide's expectedopenops-enginepod never exists; update all engine references in this guide as part of the rename.
docs/DEPLOY_TO_AWS_EKS_FARGATE.md:680 - After this registry change, the Fargate values below still configure
engine/openops-engineand later diagnostics target componentengine. Those keys no longer configure the chart, so users following the guide silently get default worker settings and invalid log/health commands; migrate the guide toworker.
- Files reviewed: 32/32 changed files
- Comments generated: 0 new
- Review effort level: Lite
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Several unresolved functional, security, rollout, configuration, and documentation issues remain.
Review effort: Lite
Findings: 2
Open (7)
http://localhost.example.comand…AZURE_ACR_RELEASE_CLIENT_ID, but the release contract documented in…namespaceSelector: {}only selects pods in Kubernetes namespaces; it does not allow traffic to…networkPolicy.enabled(the default), this new MCP egress rule is not sufficient to reach the…global.versiondescription still says it applies to app and engine images, even…
ravikiranvm
approved these changes
Sep 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of OPS-4456