Skip to content

feat(helm): Expose serviceAccountName on the worker deployment template. - #420

Merged
20001020ycx merged 1 commit into
y-scope:mainfrom
20001020ycx:feat/2026-07-29-worker-service-account
Jul 30, 2026
Merged

feat(helm): Expose serviceAccountName on the worker deployment template.#420
20001020ycx merged 1 commit into
y-scope:mainfrom
20001020ycx:feat/2026-07-29-worker-service-account

Conversation

@20001020ycx

@20001020ycx 20001020ycx commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Description

The worker Deployment doesn't set a serviceAccountName, so worker pods always run under the namespace's default ServiceAccount. On cloud deployments this blocks identity-based auth — e.g., on EKS, IRSA grants an IAM role through annotations on a ServiceAccount, so a workload that can't select a ServiceAccount can't use keyless S3 access.

This exposes an optional spiderConfig.worker.service_account_name value, rendered through tpl like the existing extra_envs/extra_volumes/extra_volume_mounts hooks, so a parent chart can pass a templated name. The immediate consumer is CLP's chart, which embeds this chart as a subchart and needs the worker to run under the same ServiceAccount as its own workers (compression-worker-deployment.yaml#L24); see the review discussion in y-scope/clp#2418.

When the value is empty (the default), the field is omitted and the rendered Deployment is unchanged.

Also bumps the chart version to 0.1.6.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • helm lint passes.
  • helm template with default values renders the worker Deployment without a serviceAccountName (unchanged behavior).
  • --set spiderConfig.worker.service_account_name=my-sa renders serviceAccountName: "my-sa".
  • Setting the value to {{ .Release.Name }}-clp-service-account (as a parent chart would) renders serviceAccountName: "test-clp-service-account" for a release named test.

Summary by CodeRabbit

  • New Features

    • Added configurable ServiceAccount support for worker deployments (optionally set a specific ServiceAccount, otherwise the default is used).
    • Introduced configurable log level settings for liveness, scheduler, and storage components.
  • Chores

    • Bumped the Helm chart metadata version to 0.1.8.

@20001020ycx
20001020ycx requested review from a team and sitaowang1998 as code owners July 29, 2026 20:59
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The Helm chart adds an optional templated worker ServiceAccount name, introduces logging-level defaults for liveness, scheduler, and storage configuration, and increments the chart version from 0.1.7 to 0.1.8.

Changes

Worker ServiceAccount configuration

Layer / File(s) Summary
Worker ServiceAccount wiring
tools/deployment/spider-helm/values.yaml, tools/deployment/spider-helm/templates/worker-deployment.yaml
Adds an optional worker.service_account_name value and conditionally renders the evaluated value as the worker Deployment’s serviceAccountName.
Chart version update
tools/deployment/spider-helm/Chart.yaml
Updates the Helm chart version from 0.1.7 to 0.1.8.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: junhaoliao, hoophalab

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: exposing worker serviceAccountName in the Helm template.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

@20001020ycx
20001020ycx force-pushed the feat/2026-07-29-worker-service-account branch from 69307c0 to 2a30602 Compare July 29, 2026 21:09
sitaowang1998
sitaowang1998 previously approved these changes Jul 29, 2026
@20001020ycx
20001020ycx force-pushed the feat/2026-07-29-worker-service-account branch from 2a30602 to b5d1b86 Compare July 30, 2026 19:27

@LinZhihao-723 LinZhihao-723 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc, is it better to use:

feat(helm): Expose `serviceAccountName` on the worker deployment template.

Like I don't understand why it's "Deployment` a capital 'D'.

@20001020ycx 20001020ycx changed the title feat(helm): Expose serviceAccountName on the worker Deployment. feat(helm): Expose serviceAccountName on the worker deployment template. Jul 30, 2026
@20001020ycx
20001020ycx merged commit 2ce711a into y-scope:main Jul 30, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants