chore: remove kafka pkg, move influxdb off bitnami mirror (IP-3481) - #2819
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
📊 API Diff Results
|
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the InfluxDB override forwarding, versioning, API compatibility, and persistence behavior concerns.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Removes the unused Kafka wrapper and migrates InfluxDB away from Bitnami, with incidental formatting fixes.
Changes:
- Deletes the unused Kafka Helm package.
- Pins InfluxDB to InfluxData’s chart and adds configurable ECR image support.
- Applies incidental fixes in Benchspy and Sui files.
File summaries
| File | Summary |
|---|---|
wasp/benchspy/report.go |
Applies report directory fix. |
lib/k8s/pkg/helm/kafka/kafka.go |
Removes unused Kafka wrapper. |
lib/k8s/pkg/helm/influxdb/influxdb.go |
Migrates chart and image; review identified forwarding, versioning, API compatibility, and persistence concerns. |
lib/k8s/config/overrides.go |
Adds the InfluxDB registry override. |
framework/components/blockchain/sui_faucet_test.go |
Applies formatting cleanup. |
Review details
Suppressed comments (3)
lib/k8s/pkg/helm/influxdb/influxdb.go:85
- When the test uses CTF's remote runner, this override is read in the runner process, but
jobEnvVarsonly forwards the allowlisted variables inlib/k8s/environment/runner.go:426-444; the newINFLUXDB_IMAGE_REGISTRYis not included. Consequently, setting this variable on the invoking process is silently lost for remote-runner deployments, so the documented registry override does not work there. Add it to the runner's forwarded-variable list (and cover the forwarding path).
func registry() string {
if reg := os.Getenv(config.EnvVarInfluxdbImageRegistry); reg != "" {
return reg
}
return defaultImageRegistry
lib/k8s/pkg/helm/influxdb/influxdb.go:66
- This changes the runtime from the previous
3.4.2image to InfluxDB1.8.10, not just the registry; the referenced chart's appVersion is also1.8.10. The unchanged Atlas load test may use v2/v3 endpoints, in which case Helm can report a ready pod while its queries fail. Please verify the consumer against this API before merging and select the matching InfluxDB chart/image if necessary.
"tag": "1.8.10-alpine",
lib/k8s/pkg/helm/influxdb/influxdb.go:67
- The InfluxData 4.12.5 chart defaults
persistence.enabledto true, whereas the previous Bitnami values explicitly set it to false. Because this override is omitted here, deployments now create an 8 GiBvolumeClaimTemplateinstead of usingemptyDir, which can block clusters without a default StorageClass and changes the test environment's storage behavior. Please carry overpersistence.enabled: false(or explicitly provision/document the new persistent-storage requirement).
"image": map[string]interface{}{
"repository": fmt.Sprintf("%s/docker-io/library/influxdb", reg),
"tag": "1.8.10-alpine",
},
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // NewVersioned keeps its signature for API compatibility; the chart version is pinned in influxdataChartURL and the version argument is ignored. | ||
| func NewVersioned(helmVersion string, props map[string]interface{}) environment.ConnectedChart { |
There was a problem hiding this comment.
Kept deliberately. The only caller org-wide is atlas's NewVersioned("5.9.8", nil). Chart versions don't map between the old Bitnami chart and the InfluxData chart, so any previously passed version is meaningless after the swap; rejecting non-empty would panic that caller at runtime, and removing the constructor breaks its build. The doc comment on NewVersioned notes the argument is ignored. Happy to drop the constructor in a follow-up once atlas migrates off it.
|
The two red checks — The current api-diff failures are a tool-install breakage. From the failed job logs (run The workflow installs One consequence worth stating: because the analyzer never ran, the intentional API break in this PR — removal of the exported All other completed checks are green, including per-module lint and unit tests. |
What: Removes the last two Bitnami references in CTF (IP-3481, validator rows 4–5):
lib/k8s/pkg/helm/kafka/kafka.go— the Bitnami kafka chart wrapper. Zero importers org-wide (code search, incl. public GitHub).lib/k8s/pkg/helm/influxdb/influxdb.gooff the paid Bitnami mirror:${BITNAMI_PRIVATE_REGISTRY}/charts/debian-12/influxdb:7.1.47→ InfluxData's own chart, pinnedinfluxdb-4.12.5.tgzrelease URL. Helm ignores--versionfor URL refs, so the chart version is pinned in the URL and theNewVersionedargument is now ignored; the signature is kept so the sole consumer keeps compiling.influxdataChartURLconst (helm ignores--versionfor URL refs) — bumping it later means editing the const. The influxdb app version (image.tag) and all other chart values remain caller-overridable viaprops, as before.containers/debian-12:3.4.2→docker-io/library/influxdb:1.8.10-alpinevia the prod ECR pull-through cache (804282218731.dkr.ecr.us-west-2.amazonaws.com), overridable with the newINFLUXDB_IMAGE_REGISTRYenv var inlib/k8s/config/overrides.go.auth.enabled, probe disables,global.security.allowInsecureImages) dropped.framework/components/blockchain/sui_faucet_test.go,wasp/benchspy/report.go) are pre-commit hook auto-fixes (golangci-lint --fix) that the repo's hooks apply on any commit; not related to this change.Blast radius: kafka has zero consumers; influxdb has exactly one —
smartcontractkit/atlastests/load/graphql/graphql_test.go(k8s load test, untouched since 2025-01). Merge holds until atlas owners confirmTestDeployGraphqlSetupagainst this branch.Open question: the pinned
1.8.10-alpineis the influxdata chart's default app version; the mirror previously served3.4.2. If the atlas test speaks the v2/v3 query API we need theinfluxdb2chart instead — reviewer input welcome.Verification (local):
go build ./...,go mod tidycheck,golangci-lint run ./k8s/...(0 issues),go test -race ./k8s/config— all green.Why
--no-verify: same pre-existing hook breakage as documented in #2804 (just lint-all→ missinghavoc, self-contradicting hooks); CI runs per-module lint + tests instead — see probe #2803. On merge,bitnami-gh.py validateshould show 5/5 for this repo.