Default enable telemetry with default settings#567
Closed
kaixi-wang wants to merge 26 commits into
Closed
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces telemetry infrastructure for FiftyOne Teams App deployments across Docker Compose and Kubernetes platforms. It adds a bundled Redis-backed telemetry system with per-service sidecars for metrics collection, comprehensive documentation, Helm templating, and extensive test coverage. ChangesDocker Compose Telemetry Support
Kubernetes and Helm Telemetry Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tests/unit/helm/delegated-operator-instance-deployment_test.go (1)
361-373: ⚡ Quick winRemove duplicate
optionsredeclarations in the same subtest.Line 373 and Line 3132 redeclare
optionswith the same value already initialized earlier in the same closure. Reuse the existing variable to avoid shadowing and keep setup single-source.♻️ Proposed cleanup
- options := &helm.Options{SetValues: disableTelemetry(testCase.values)} + options := &helm.Options{SetValues: disableTelemetry(testCase.values)} if testCase.expected == nil { output, err := helm.RenderTemplateE(subT, options, s.chartPath, s.releaseName, s.templates) @@ } else { - options := &helm.Options{SetValues: disableTelemetry(testCase.values)} - output := helm.RenderTemplate(subT, options, s.chartPath, s.releaseName, s.templates)- options := &helm.Options{SetValues: disableTelemetry(testCase.values)} + options := &helm.Options{SetValues: disableTelemetry(testCase.values)} if testCase.values == nil { - options := &helm.Options{SetValues: disableTelemetry(testCase.values)} output, err := helm.RenderTemplateE(subT, options, s.chartPath, s.releaseName, s.templates)Also applies to: 3130-3133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/helm/delegated-operator-instance-deployment_test.go` around lines 361 - 373, The test contains duplicate redeclarations of the options variable (helm.Options with SetValues: disableTelemetry(testCase.values)) inside the same subtest closure; remove the second "options :=" shadowing and reuse the already-initialized options so there's only one declaration per subtest (refer to the options variable, helm.Options type, and disableTelemetry(testCase.values) call in the delegated-operator-instance-deployment_test.go subtest), ensuring subsequent uses just reference options instead of redeclaring it.tests/unit/helm/teams-app-deployment_test.go (1)
100-2603: ⚡ Quick winPreserve a true default telemetry-on test path in this file.
At Line 100 and throughout, forcing
disableTelemetry(...)across all subtests means the suite no longer checks the chart’s default telemetry-enabled behavior. Consider adding a small set of explicit default/on-path tests alongside these disabled-baseline assertions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/helm/teams-app-deployment_test.go` around lines 100 - 2603, The tests currently force telemetry off by calling disableTelemetry(...) for every subtest (see the usage in options := &helm.Options{SetValues: disableTelemetry(testCase.values)} across TestMetadataName, TestReplicas, TestContainerEnv, etc.), which removes coverage of the chart's default telemetry-enabled path; add explicit subtests that do not wrap values with disableTelemetry (i.e., pass testCase.values directly or nil to helm.Options) to assert the default telemetry-on behavior, or only call disableTelemetry for the specific override cases where telemetry must be disabled, updating each test table or the options construction in functions like TestContainerEnv, TestMetadataName, TestTemplateLabels, etc., so at least one case per Test* retains the unmodified/default values path.tests/unit/helm/plugins-deployment_test.go (1)
105-3004: ⚡ Quick winAdd at least one telemetry-enabled assertion path in this suite.
From Line 105 onward, all renders are forced through
disableTelemetry(...), so even “defaultValues” cases no longer validate default telemetry-on behavior. Keep these disabled-baseline tests, but add explicit telemetry-enabled cases (or a non-wrapped render path) for key assertions like container composition to preserve default-path coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/helm/plugins-deployment_test.go` around lines 105 - 3004, The suite always wraps SetValues with disableTelemetry(...) so no test exercises the telemetry-enabled default path; add explicit telemetry-enabled assertions by either adding new test cases or an alternate render path that does not call disableTelemetry (e.g., call helm.RenderTemplate / helm.RenderTemplateE with options built from testCase.values directly) in key tests such as TestContainerCount, TestContainerEnv, TestContainerImage (and any other container/composition tests) so you validate both telemetry-disabled and default telemetry-enabled behavior; ensure new cases use the same matching expectations but with telemetry left enabled.tests/unit/compose/docker-compose-internal-auth_test.go (1)
131-151: ⚡ Quick winAdd explicit image/environment assertions for GPU telemetry services.
This new GPU scenario validates service presence, but it doesn’t assert runtime contract fields (image/env) for
teams-do-gpuandteams-do-gpu-telemetry. Adding those checks will harden regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/compose/docker-compose-internal-auth_test.go` around lines 131 - 151, In the "composeDelegatedOperationsGpu" test case in tests/unit/compose/docker-compose-internal-auth_test.go, add explicit runtime contract assertions for the services "teams-do-gpu" and "teams-do-gpu-telemetry": check their runtimeContract.Image fields and check required environment variables exist (runtimeContract.Env contains expected keys/values), following the same assertion pattern used for other services in this file (e.g., the existing checks for "teams-do" or telemetry services) so the GPU scenario validates both presence and image/env runtime contract fields.tests/unit/compose/docker-compose-legacy-auth_test.go (1)
131-151: ⚡ Quick winConsider asserting GPU telemetry service image/environment contracts too.
Like internal-auth, this adds GPU service-name checks but not explicit image/env assertions for
teams-do-gpuandteams-do-gpu-telemetry. Adding them would improve parity and catch config regressions earlier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/compose/docker-compose-legacy-auth_test.go` around lines 131 - 151, The test currently verifies service names for the GPU compose set (using legacyAuthComposeDelegatedOperationsGpuFile and service list containing "teams-do-gpu" and "teams-do-gpu-telemetry") but does not assert their image and environment variable contracts; update the test to mirror the internal-auth assertions by adding explicit checks for the Docker image and required env keys/values for "teams-do-gpu" and "teams-do-gpu-telemetry" (use the same assertion helpers or patterns used for internal-auth checks in this test file), ensuring the expected image strings and critical env vars are validated against the parsed compose data (referencing the service names "teams-do-gpu" and "teams-do-gpu-telemetry" to locate where to add these assertions).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/internal-auth/env.template`:
- Around line 102-106: Update the telemetry comment to reflect the new
default-enabled behavior: remove or reword the assertion that telemetry is
"optional; requires compose.telemetry.yaml" and instead state that telemetry is
enabled by default and that FIFTYONE_TELEMETRY_REDIS_URL can be used to override
the in-stack Redis (still mentioning compose.telemetry.yaml only as an optional
overlay to customize stack telemetry). Edit the block referencing
FIFTYONE_TELEMETRY_REDIS_URL so it accurately documents the new default and how
to override it.
In `@docker/legacy-auth/env.template`:
- Around line 107-111: The telemetry comment in docker/legacy-auth/env.template
is outdated: it still references an optional compose.telemetry.yaml overlay even
though telemetry is now bundled and default-enabled. Update the block that
mentions FIFTYONE_TELEMETRY_REDIS_URL to remove references to
compose.telemetry.yaml and instead state that telemetry is
bundled/default-enabled and will use the in-stack Redis
(redis://telemetry-redis:6379) when left blank; keep the example variable name
FIFTYONE_TELEMETRY_REDIS_URL and clarify that overriding it will point telemetry
sidecars and teams services to a custom Redis URL.
In `@docs/orchestrators/configuring-kubernetes-orchestrator.md`:
- Around line 510-512: Replace the hardcoded telemetry Redis DNS in the example
env var FIFTYONE_TELEMETRY_REDIS_URL (and the other occurrence) with a clear
placeholder such as <release>-telemetry-redis.<namespace>.svc.cluster.local and
add a short note that users must substitute their actual Helm release name and
namespace; update the example values where FIFTYONE_TELEMETRY_REDIS_URL is shown
and mirror the same change for the second occurrence mentioned so the
Helm-scoped service name won’t break when copied.
In `@helm/fiftyone-teams-app/templates/_telemetry.tpl`:
- Around line 113-114: The Helm template defaults currently use
voxel51/telemetry-sidecar:latest which makes deployments non-reproducible;
change the default for .ctx.Values.telemetry.sidecar.image to a specific
immutable tag or digest (for example voxel51/telemetry-sidecar:v1.2.3 or
voxel51/telemetry-sidecar@sha256:...) and do the same for the native-sidecar
image (.ctx.Values.telemetry.nativeSidecar.image), and ensure the corresponding
imagePullPolicy (.ctx.Values.telemetry.sidecar.imagePullPolicy and native
variant) is set consistently (e.g., IfNotPresent for tagged images or Always may
be kept for digests) so the sidecar image is pinned and pulls are deterministic.
In `@tests/unit/helm/delegated-operator-job-configmap_test.go`:
- Around line 713-714: Guard direct indexing into InitContainers by asserting
its length before access: add a
s.Require().Len(job.Spec.Template.Spec.InitContainers, 1) (or
s.Require().True(len(job.Spec.Template.Spec.InitContainers) > 0)) immediately
before the line that reads job.Spec.Template.Spec.InitContainers[0] in the test
so the failure is descriptive; apply the same pattern for the other occurrence
around the lines that reference InitContainers[0] (the assertions that check
TARGET_CONTAINER/custom-executor).
In `@utils/validate-docker-pulls.sh`:
- Around line 17-20: The EXPECTED_IMAGES array normalization currently appends
:${appVersion} to every voxel51/* entry which overwrites explicitly tagged
internal images like voxel51/telemetry-sidecar:latest; update the normalization
loop that processes EXPECTED_IMAGES so it checks each image string for an
existing tag (presence of ':' after the repository/name) and only appends
:${appVersion} when no tag is present (leave entries that already contain a tag
untouched). Locate the normalization logic that references EXPECTED_IMAGES and
the code block handling voxel51/* and change it to perform a conditional append
based on whether the image string already contains a tag.
---
Nitpick comments:
In `@tests/unit/compose/docker-compose-internal-auth_test.go`:
- Around line 131-151: In the "composeDelegatedOperationsGpu" test case in
tests/unit/compose/docker-compose-internal-auth_test.go, add explicit runtime
contract assertions for the services "teams-do-gpu" and
"teams-do-gpu-telemetry": check their runtimeContract.Image fields and check
required environment variables exist (runtimeContract.Env contains expected
keys/values), following the same assertion pattern used for other services in
this file (e.g., the existing checks for "teams-do" or telemetry services) so
the GPU scenario validates both presence and image/env runtime contract fields.
In `@tests/unit/compose/docker-compose-legacy-auth_test.go`:
- Around line 131-151: The test currently verifies service names for the GPU
compose set (using legacyAuthComposeDelegatedOperationsGpuFile and service list
containing "teams-do-gpu" and "teams-do-gpu-telemetry") but does not assert
their image and environment variable contracts; update the test to mirror the
internal-auth assertions by adding explicit checks for the Docker image and
required env keys/values for "teams-do-gpu" and "teams-do-gpu-telemetry" (use
the same assertion helpers or patterns used for internal-auth checks in this
test file), ensuring the expected image strings and critical env vars are
validated against the parsed compose data (referencing the service names
"teams-do-gpu" and "teams-do-gpu-telemetry" to locate where to add these
assertions).
In `@tests/unit/helm/delegated-operator-instance-deployment_test.go`:
- Around line 361-373: The test contains duplicate redeclarations of the options
variable (helm.Options with SetValues: disableTelemetry(testCase.values)) inside
the same subtest closure; remove the second "options :=" shadowing and reuse the
already-initialized options so there's only one declaration per subtest (refer
to the options variable, helm.Options type, and
disableTelemetry(testCase.values) call in the
delegated-operator-instance-deployment_test.go subtest), ensuring subsequent
uses just reference options instead of redeclaring it.
In `@tests/unit/helm/plugins-deployment_test.go`:
- Around line 105-3004: The suite always wraps SetValues with
disableTelemetry(...) so no test exercises the telemetry-enabled default path;
add explicit telemetry-enabled assertions by either adding new test cases or an
alternate render path that does not call disableTelemetry (e.g., call
helm.RenderTemplate / helm.RenderTemplateE with options built from
testCase.values directly) in key tests such as TestContainerCount,
TestContainerEnv, TestContainerImage (and any other container/composition tests)
so you validate both telemetry-disabled and default telemetry-enabled behavior;
ensure new cases use the same matching expectations but with telemetry left
enabled.
In `@tests/unit/helm/teams-app-deployment_test.go`:
- Around line 100-2603: The tests currently force telemetry off by calling
disableTelemetry(...) for every subtest (see the usage in options :=
&helm.Options{SetValues: disableTelemetry(testCase.values)} across
TestMetadataName, TestReplicas, TestContainerEnv, etc.), which removes coverage
of the chart's default telemetry-enabled path; add explicit subtests that do not
wrap values with disableTelemetry (i.e., pass testCase.values directly or nil to
helm.Options) to assert the default telemetry-on behavior, or only call
disableTelemetry for the specific override cases where telemetry must be
disabled, updating each test table or the options construction in functions like
TestContainerEnv, TestMetadataName, TestTemplateLabels, etc., so at least one
case per Test* retains the unmodified/default values path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99f93060-1e72-4cc3-ba40-efe17b79d169
⛔ Files ignored due to path filters (22)
docker/common-services.yamlis excluded by!**/*.yamldocker/internal-auth/compose.dedicated-plugins.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.yamlis excluded by!**/*.yamldocker/internal-auth/compose.plugins.yamlis excluded by!**/*.yamldocker/internal-auth/compose.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.dedicated-plugins.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.plugins.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/api-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/api-role.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/app-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/delegated-operator-instance-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/delegated-operator-job-configmap.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/plugins-deployment.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-redis.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/telemetry-rolebinding.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/values.schema.jsonis excluded by!**/*.jsonhelm/fiftyone-teams-app/values.yamlis excluded by!**/*.yamltests/fixtures/docker/compose.override.yamlis excluded by!**/*.yaml
📒 Files selected for processing (24)
docker/README.mddocker/docs/configuring-telemetry.mddocker/internal-auth/env.templatedocker/legacy-auth/env.templatedocs/orchestrators/configuring-kubernetes-orchestrator.mdhelm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/README.md.gotmplhelm/fiftyone-teams-app/templates/_do_targets.tplhelm/fiftyone-teams-app/templates/_helpers.tplhelm/fiftyone-teams-app/templates/_telemetry.tpltests/unit/compose/docker-compose-internal-auth_test.gotests/unit/compose/docker-compose-legacy-auth_test.gotests/unit/helm/api-deployment_test.gotests/unit/helm/api-role_test.gotests/unit/helm/app-deployment_test.gotests/unit/helm/common_test.gotests/unit/helm/delegated-operator-instance-deployment_test.gotests/unit/helm/delegated-operator-job-configmap_test.gotests/unit/helm/plugins-deployment_test.gotests/unit/helm/teams-app-deployment_test.gotests/unit/helm/telemetry-redis_test.gotests/unit/helm/telemetry-rolebinding_test.gotests/unit/helm/yaml_helpers.goutils/validate-docker-pulls.sh
Comment on lines
+102
to
+106
| # ─── Telemetry (optional; requires compose.telemetry.yaml overlay) ───── | ||
| # Redis URL used by the telemetry sidecars and the teams-api / teams-app | ||
| # services. Leave blank to use the default in-stack Redis defined by | ||
| # compose.telemetry.yaml (redis://telemetry-redis:6379). | ||
| # FIFTYONE_TELEMETRY_REDIS_URL= |
Contributor
There was a problem hiding this comment.
Telemetry guidance here contradicts the new default-enabled behavior.
Line 102 through Line 106 still says telemetry is optional and requires compose.telemetry.yaml, which conflicts with the new default-enabled flow described elsewhere in this PR.
Suggested doc fix
-# ─── Telemetry (optional; requires compose.telemetry.yaml overlay) ─────
+# ─── Telemetry (enabled by default) ─────
# Redis URL used by the telemetry sidecars and the teams-api / teams-app
-# services. Leave blank to use the default in-stack Redis defined by
-# compose.telemetry.yaml (redis://telemetry-redis:6379).
+# services. Leave blank to use the default in-stack Redis
+# (redis://telemetry-redis:6379).📝 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.
Suggested change
| # ─── Telemetry (optional; requires compose.telemetry.yaml overlay) ───── | |
| # Redis URL used by the telemetry sidecars and the teams-api / teams-app | |
| # services. Leave blank to use the default in-stack Redis defined by | |
| # compose.telemetry.yaml (redis://telemetry-redis:6379). | |
| # FIFTYONE_TELEMETRY_REDIS_URL= | |
| # ─── Telemetry (enabled by default) ───── | |
| # Redis URL used by the telemetry sidecars and the teams-api / teams-app | |
| # services. Leave blank to use the default in-stack Redis | |
| # (redis://telemetry-redis:6379). | |
| # FIFTYONE_TELEMETRY_REDIS_URL= |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/internal-auth/env.template` around lines 102 - 106, Update the
telemetry comment to reflect the new default-enabled behavior: remove or reword
the assertion that telemetry is "optional; requires compose.telemetry.yaml" and
instead state that telemetry is enabled by default and that
FIFTYONE_TELEMETRY_REDIS_URL can be used to override the in-stack Redis (still
mentioning compose.telemetry.yaml only as an optional overlay to customize stack
telemetry). Edit the block referencing FIFTYONE_TELEMETRY_REDIS_URL so it
accurately documents the new default and how to override it.
Comment on lines
+107
to
+111
| # ─── Telemetry (optional; requires compose.telemetry.yaml overlay) ───── | ||
| # Redis URL used by the telemetry sidecars and the teams-api / teams-app | ||
| # services. Leave blank to use the default in-stack Redis defined by | ||
| # compose.telemetry.yaml (redis://telemetry-redis:6379). | ||
| # FIFTYONE_TELEMETRY_REDIS_URL= |
Contributor
There was a problem hiding this comment.
Telemetry guidance is outdated relative to this PR’s default behavior.
Line 107 through Line 111 still references an optional compose.telemetry.yaml overlay, but telemetry is now described as bundled/default-enabled.
Suggested doc fix
-# ─── Telemetry (optional; requires compose.telemetry.yaml overlay) ─────
+# ─── Telemetry (enabled by default) ─────
# Redis URL used by the telemetry sidecars and the teams-api / teams-app
-# services. Leave blank to use the default in-stack Redis defined by
-# compose.telemetry.yaml (redis://telemetry-redis:6379).
+# services. Leave blank to use the default in-stack Redis
+# (redis://telemetry-redis:6379).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker/legacy-auth/env.template` around lines 107 - 111, The telemetry
comment in docker/legacy-auth/env.template is outdated: it still references an
optional compose.telemetry.yaml overlay even though telemetry is now bundled and
default-enabled. Update the block that mentions FIFTYONE_TELEMETRY_REDIS_URL to
remove references to compose.telemetry.yaml and instead state that telemetry is
bundled/default-enabled and will use the in-stack Redis
(redis://telemetry-redis:6379) when left blank; keep the example variable name
FIFTYONE_TELEMETRY_REDIS_URL and clarify that overriding it will point telemetry
sidecars and teams services to a custom Redis URL.
Comment on lines
+510
to
+512
| - name: FIFTYONE_TELEMETRY_REDIS_URL | ||
| value: redis://telemetry-redis.fiftyone.svc.cluster.local:6379 | ||
| - name: FIFTYONE_DATABASE_URI |
Contributor
There was a problem hiding this comment.
Use a placeholder for Redis service DNS to avoid Helm naming mismatch.
Line 511 and Line 665 hardcode telemetry-redis.... In Helm deployments, the service name is release-scoped, so this example can fail if copied as-is. Please switch to a placeholder (for example, <release>-telemetry-redis.<namespace>.svc.cluster.local) and call out that users must use their actual service DNS.
Also applies to: 664-666
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/orchestrators/configuring-kubernetes-orchestrator.md` around lines 510 -
512, Replace the hardcoded telemetry Redis DNS in the example env var
FIFTYONE_TELEMETRY_REDIS_URL (and the other occurrence) with a clear placeholder
such as <release>-telemetry-redis.<namespace>.svc.cluster.local and add a short
note that users must substitute their actual Helm release name and namespace;
update the example values where FIFTYONE_TELEMETRY_REDIS_URL is shown and mirror
the same change for the second occurrence mentioned so the Helm-scoped service
name won’t break when copied.
Comment on lines
+113
to
+114
| image: {{ .ctx.Values.telemetry.sidecar.image | default "voxel51/telemetry-sidecar:latest" | quote }} | ||
| imagePullPolicy: {{ .ctx.Values.telemetry.sidecar.imagePullPolicy | default "Always" }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify telemetry defaults and image mutability in chart files
rg -n --iglob '*values*.yaml' 'telemetry:\s*$|enabled:\s*true|sidecar:|image:' helm/fiftyone-teams-app
rg -n 'telemetry-sidecar:latest|telemetry\.sidecar\.image' helm/fiftyone-teams-app/templates helm/fiftyone-teams-app/README.mdRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 3128
🏁 Script executed:
# Read telemetry template to see context of both image defaults
sed -n '100,125p' helm/fiftyone-teams-app/templates/_telemetry.tpl
sed -n '130,150p' helm/fiftyone-teams-app/templates/_telemetry.tpl
# Read telemetry values section
sed -n '1657,1700p' helm/fiftyone-teams-app/values.yamlRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 3122
Pin telemetry sidecar image to an immutable tag or digest.
Lines 113 and 139 in helm/fiftyone-teams-app/templates/_telemetry.tpl both default to voxel51/telemetry-sidecar:latest. With telemetry enabled by default and imagePullPolicy: Always, this causes non-reproducible deployments—each pull may fetch a different image version. Use a specific version tag (e.g., voxel51/telemetry-sidecar:v1.2.3) or a digest to ensure consistent behavior.
Also applies to: lines 139-140 (native-sidecar variant).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/fiftyone-teams-app/templates/_telemetry.tpl` around lines 113 - 114, The
Helm template defaults currently use voxel51/telemetry-sidecar:latest which
makes deployments non-reproducible; change the default for
.ctx.Values.telemetry.sidecar.image to a specific immutable tag or digest (for
example voxel51/telemetry-sidecar:v1.2.3 or
voxel51/telemetry-sidecar@sha256:...) and do the same for the native-sidecar
image (.ctx.Values.telemetry.nativeSidecar.image), and ensure the corresponding
imagePullPolicy (.ctx.Values.telemetry.sidecar.imagePullPolicy and native
variant) is set consistently (e.g., IfNotPresent for tagged images or Always may
be kept for digests) so the sidecar image is pinned and pulls are deterministic.
Comment on lines
+713
to
+714
| sidecar := job.Spec.Template.Spec.InitContainers[0] | ||
| s.True(envContains(sidecar.Env, "TARGET_CONTAINER", "custom-executor"), "User-set TARGET_CONTAINER must win") |
Contributor
There was a problem hiding this comment.
Guard InitContainers[0] access with an explicit length assertion.
These direct indexes can panic if rendering changes. Add Require().Len(...) first so failures stay descriptive.
🛡️ Suggested test hardening
func(subT gruntworkTesting.TestingT, job batchv1.Job) {
- sidecar := job.Spec.Template.Spec.InitContainers[0]
+ s.Require().Len(job.Spec.Template.Spec.InitContainers, 1, "Expected one native sidecar init container")
+ sidecar := job.Spec.Template.Spec.InitContainers[0]
s.True(envContains(sidecar.Env, "TARGET_CONTAINER", "custom-executor"), "User-set TARGET_CONTAINER must win")
s.False(envContains(sidecar.Env, "TARGET_CONTAINER", "executor"), "Auto-inject must not also append TARGET_CONTAINER=executor")
},
@@
func(subT gruntworkTesting.TestingT, job batchv1.Job) {
podSpec := job.Spec.Template.Spec
s.False(volumeContains(podSpec.Volumes, "telemetry-socket"), "Non-executor sidecar must not auto-inject telemetry-socket volume")
- sidecar := podSpec.InitContainers[0]
+ s.Require().Len(podSpec.InitContainers, 1, "Expected one native sidecar init container")
+ sidecar := podSpec.InitContainers[0]
s.False(volumeMountContains(sidecar.VolumeMounts, "telemetry-socket", "/tmp/telemetry"), "Non-executor sidecar must not auto-inject volumeMount")Also applies to: 730-731
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/helm/delegated-operator-job-configmap_test.go` around lines 713 -
714, Guard direct indexing into InitContainers by asserting its length before
access: add a s.Require().Len(job.Spec.Template.Spec.InitContainers, 1) (or
s.Require().True(len(job.Spec.Template.Spec.InitContainers) > 0)) immediately
before the line that reads job.Spec.Template.Spec.InitContainers[0] in the test
so the failure is descriptive; apply the same pattern for the other occurrence
around the lines that reference InitContainers[0] (the assertions that check
TARGET_CONTAINER/custom-executor).
Comment on lines
17
to
20
| EXPECTED_IMAGES=( | ||
| "docker.io/busybox:stable-glibc" # For initContainers | ||
| "docker.io/redis:7-alpine" # For telemetry redis (telemetry.enabled defaults to true) | ||
| "voxel51/fiftyone-app" |
Contributor
There was a problem hiding this comment.
Handle explicitly tagged internal images during expected-image normalization.
voxel51/telemetry-sidecar:latest is rendered by Helm (as shown by the pipeline failure), but the current loop appends :${appVersion} to every voxel51/* entry. That blocks adding tagged internal images and keeps this validation failing.
Proposed fix
EXPECTED_IMAGES=(
"docker.io/busybox:stable-glibc" # For initContainers
"docker.io/redis:7-alpine" # For telemetry redis (telemetry.enabled defaults to true)
+ "voxel51/telemetry-sidecar:latest" # For telemetry sidecar
"voxel51/fiftyone-app"
"voxel51/fiftyone-app-gpt"
"voxel51/fiftyone-app-torch"
"voxel51/fiftyone-teams-api"
@@
for img in "${EXPECTED_IMAGES[@]}"; do
- if [[ ${img} =~ "voxel51/" ]]; then
+ if [[ "${img}" == voxel51/* && "${img}" != *:* ]]; then
# Only add a tag for our organizations images, not
# publicly available ones our chart may reference
img_with_tag="${img}:${expected_tag}"
else
img_with_tag="${img}"Also applies to: 105-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/validate-docker-pulls.sh` around lines 17 - 20, The EXPECTED_IMAGES
array normalization currently appends :${appVersion} to every voxel51/* entry
which overwrites explicitly tagged internal images like
voxel51/telemetry-sidecar:latest; update the normalization loop that processes
EXPECTED_IMAGES so it checks each image string for an existing tag (presence of
':' after the repository/name) and only appends :${appVersion} when no tag is
present (leave entries that already contain a tag untouched). Locate the
normalization logic that references EXPECTED_IMAGES and the code block handling
voxel51/* and change it to perform a conditional append based on whether the
image string already contains a tag.
…v2.19.0-rc.5 api:v2.19.0rc6 app:v2.19.0rc6 cas:v2.19.0-rc.5
…lt-enable' into kacey/sidecar-default-enable
…v2.19.0-rc.6 api:v2.19.0rc7 app:v2.19.0rc7 cas:v2.19.0-rc.6
4 tasks
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
2 participants
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.
Rationale
Testing ability to set default configs so that it just works without additional helm chart mods
Review Priority
Changes
Checklist
Testing