fix: Expose wandb container images #364
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughSwitches the wandb Terraform module source to a GitHub module, adds image configuration blocks for multiple components, introduces new image-related variables, and passes a new controller_image_repository input. Updates parquet to include an image entry and adjusts a module move reference. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.tf (1)
372-383: Emit tag only when set to avoid tag: null in values.If local.spec is handed to a module that serializes to YAML, "tag = null" may render as tag: null. Prefer merging the tag key only when a tag is provided.
Apply this pattern (here and in other image blocks):
- app = { + app = { internalJWTMap = [ { "subject" = "system:serviceaccount:default:${local.weave_trace_sa_name}", "issuer" = "https://${module.app_eks.aws_iam_openid_connect_provider}" } ] - image = { - repository = var.wandb_local_image_repository - tag = var.wandb_local_image_tag - } + image = merge( + { repository = var.wandb_local_image_repository }, + var.wandb_local_image_tag == null ? {} : { tag = var.wandb_local_image_tag } + ) }Repeat the same merge() approach for console, weave, parquet, settingsMigrationJob, api, glue, executor image maps below.
🧹 Nitpick comments (6)
variables.tf (2)
679-689: Console image vars mirror local; same null-tag caveat.Same suggestion as above for wandb_console_image_repository/tag regarding null serialization and avoiding repo with an embedded tag when tag is set.
691-701: Megabinary image vars: confirm the intended default registry and digest support.
- If customers will use digests (e.g., @sha256:...), document whether tag accepts digest strings or if repository should accept digest notation. That impacts validation strategy.
- Consider adding an example showing how to point to a private ECR/GCR/ACR repository.
main.tf (4)
413-427: Weave image: comment mentions future switch; consider centralizing image locals.You could reduce duplication and make future switches simpler by introducing locals for the three image groups (local, console, megabinary) and reusing them.
Example:
+locals { + image_local = merge({ repository = var.wandb_local_image_repository }, var.wandb_local_image_tag == null ? {} : { tag = var.wandb_local_image_tag }) + image_console = merge({ repository = var.wandb_console_image_repository }, var.wandb_console_image_tag == null ? {} : { tag = var.wandb_console_image_tag }) + image_megabinary = merge({ repository = var.wandb_megabinary_image_repository }, var.wandb_megabinary_image_tag == null ? {} : { tag = var.wandb_megabinary_image_tag }) +} ... - image = { - # After chart operator-wandb 0.32.9 replace with wandb/weave-python - # repository: wandb/weave-python - repository = var.wandb_local_image_repository - tag = var.wandb_local_image_tag - } + image = local.image_local
429-437: Parquet image: confirm intended repository vs. megabinary transition.Comment suggests a future switch to megabinary; today it uses the local image. Verify this matches the current chart behavior. If parquet must run megabinary already in some versions, consider gating by operator/chart version or adding a variable to explicitly pick the source to avoid surprises during upgrades.
439-465: Megabinary blocks (settingsMigrationJob, api, glue, executor): reduce duplication.Four identical image maps. Favor a single local.image_megabinary used in all four places (see earlier suggestion). This will also make future migrations easier.
490-495: New controller_image_repository input: verify upstream compatibility.Confirm terraform-helm-wandb’s expose_wandb_container_images branch actually exposes controller_image_repository and that default values here match the upstream schema. If not, plans will fail at apply time.
I can cross-check the upstream module input map once PR #9’s interface is finalized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
main.tf(4 hunks)variables.tf(2 hunks)
🔇 Additional comments (2)
variables.tf (1)
35-40: Expose controller image repo: good addition; consider pinning defaults and docs.The new controller_image_repository variable looks fine and aligns with the main.tf usage. Two follow-ups:
- Add a brief note in README/variables docs about how this interacts with private registries and imagePullSecrets (if supported upstream).
- Confirm the default "wandb/controller" matches the image name the upstream module expects for the selected controller_image_tag (1.20.0). Otherwise, explicitly document the compatible pairs.
main.tf (1)
500-502: State move block: good; ensure minimum Terraform version is set.The moved block is appropriate to drop the [0] index. Ensure versions.tf (or required_version) enforces Terraform >= 1.1 (moved blocks support). If not present, add it.
| console = { | ||
| extraEnv = { | ||
| "BUCKET_ACCESS_IDENTITY" = module.app_eks.node_role.arn | ||
| } | ||
| image = { | ||
| repository = var.wandb_console_image_repository | ||
| tag = var.wandb_console_image_tag | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Console image: same conditional-tag pattern applies.
See prior comment; use merge() to avoid serializing null tags.
🤖 Prompt for AI Agents
In main.tf around lines 385 to 393, the console.image block currently sets tag
directly which serializes nulls; change it to use merge() so the tag key is only
present when var.wandb_console_image_tag is non-null. Replace the image map with
something like merge({ repository = var.wandb_console_image_repository },
var.wandb_console_image_tag != null ? { tag = var.wandb_console_image_tag } :
{}) so null tags are not serialized.
main.tf
Outdated
| # source = "wandb/wandb/helm" | ||
| # version = "3.0.0" | ||
| source = "github.com/wandb/terraform-helm-wandb?ref=expose_wandb_container_images" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Module source points to a feature branch; pin to an immutable ref.
Referencing github.com/wandb/terraform-helm-wandb?ref=expose_wandb_container_images ties production to an unmerged, mutable branch and creates supply-chain drift risk. Pin to a commit SHA and add a TODO to move to a tagged release once wandb/terraform-helm-wandb#9 merges.
Proposed change:
- source = "github.com/wandb/terraform-helm-wandb?ref=expose_wandb_container_images"
+ # TODO: switch to a tagged release after PR #9 merges
+ source = "github.com/wandb/terraform-helm-wandb?ref=<commit-sha>"Also, ensure CI runs terraform init -upgrade only when intentionally updating modules to avoid accidental ref drift.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # source = "wandb/wandb/helm" | |
| # version = "3.0.0" | |
| source = "github.com/wandb/terraform-helm-wandb?ref=expose_wandb_container_images" | |
| # source = "wandb/wandb/helm" | |
| # version = "3.0.0" | |
| # TODO: switch to a tagged release after PR #9 merges | |
| source = "github.com/wandb/terraform-helm-wandb?ref=<commit-sha>" |
🤖 Prompt for AI Agents
In main.tf around lines 478 to 481, the module source currently points to a
mutable feature branch (?ref=expose_wandb_container_images); replace the branch
ref with an immutable commit SHA for that PR (pin to the exact commit hash), add
a nearby TODO comment to switch to the official tagged release once
https://github.com/wandb/terraform-helm-wandb/pull/9 merges, and update
documentation/CI notes so terraform init -upgrade is only executed intentionally
(e.g., gated by a special pipeline flag or manual step) to prevent accidental
module ref drift.
| variable "wandb_local_image_repository" { | ||
| type = string | ||
| description = "wandb/local image repository" | ||
| default = "wandb/local" | ||
| } | ||
|
|
||
| variable "wandb_local_image_tag" { | ||
| type = string | ||
| description = "wandb/local image tag" | ||
| default = null | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Null tags may serialize as tag: null; guard to omit keys when unset.
Using default = null for wandb_local_image_tag is convenient, but if the downstream module renders YAML directly, it can result in tag: null instead of omitting the key, which some charts don’t tolerate. Consider conditioning tag emission (see main.tf comments for a merge() pattern). Also, ensure repository values that already include a tag (repo:tag) are not combined with tag again.
Apply this optional validation to prevent “repo already includes tag” when tag is set:
variable "wandb_local_image_repository" {
type = string
description = "wandb/local image repository"
default = "wandb/local"
}
variable "wandb_local_image_tag" {
type = string
description = "wandb/local image tag"
default = null
+ validation {
+ condition = var.wandb_local_image_tag == null || !can(regex(":[^/]+$", var.wandb_local_image_repository))
+ error_message = "Do not include a tag in wandb_local_image_repository when wandb_local_image_tag is set."
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| variable "wandb_local_image_repository" { | |
| type = string | |
| description = "wandb/local image repository" | |
| default = "wandb/local" | |
| } | |
| variable "wandb_local_image_tag" { | |
| type = string | |
| description = "wandb/local image tag" | |
| default = null | |
| } | |
| variable "wandb_local_image_repository" { | |
| type = string | |
| description = "wandb/local image repository" | |
| default = "wandb/local" | |
| } | |
| variable "wandb_local_image_tag" { | |
| type = string | |
| description = "wandb/local image tag" | |
| default = null | |
| validation { | |
| condition = var.wandb_local_image_tag == null || !can(regex(":[^/]+$", var.wandb_local_image_repository)) | |
| error_message = "Do not include a tag in wandb_local_image_repository when wandb_local_image_tag is set." | |
| } | |
| } |
🤖 Prompt for AI Agents
In variables.tf around lines 667–677, wandb_local_image_tag is defaulted to null
which can serialize to "tag: null" downstream and you also risk double-tagging
if wandb_local_image_repository already contains a tag; update handling by: keep
or change the default but add a validation block that errors when a tag is set
and the repository string already contains a tag delimiter (":" or "@"), and
update the template/merge logic that renders the image to only emit the tag key
when the variable is non-empty/non-null and the repository does not already
include a tag (use conditional merge() pattern described in main.tf to omit the
tag key when unset).
This change, dependent on the PR, has the same purpose: to expose the image repository configuration for customers restricted from pulling images from public repositories.
I created a fix because the feature was introduced in the PR, but I'm happy to recreate the PR as another feature.
Summary by CodeRabbit
New Features
Improvements
Chores