chore(release): v2.21.0#593
Conversation
chore: bump release 2.21.0
…v2.21.0-rc.2 api:v2.21.0rc3 app:v2.21.0rc3 cas:v2.21.0-rc.2
…v2.21.0-rc.3 api:v2.21.0rc4 app:v2.21.0rc4 cas:v2.21.0-rc.3
…v2.21.0-rc.4 api:v2.21.0rc5 app:v2.21.0rc5 cas:v2.21.0-rc.4
WalkthroughThis PR bumps the FiftyOne Enterprise release version from 2.20.0 to 2.21.0 across the Makefile, docs, and test fixtures/expectations, updates integration test service startup/health check assertions, adds a new post-installation recommended configuration doc, and extends the Helm telemetry sidecar template with extra env vars, extra volumes, and RBAC-disabled handling plus new unit tests. ChangesVersion Bump to 2.21.0
Recommended Post-Install Configuration Docs
Telemetry Sidecar Extra Env/Volumes and RBAC Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
feat(sidecar): support extraVolumes
fix: make auto rbac create optional
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6.2.0 to 6.3.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6.2.0...v6.3.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…privileged is true
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Kacey <kacey@voxel51.com>
…decar-detection' into kacey/fix-k8-gpu-sidecar-detection
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 6.2.0 to 6.3.0. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v6.2.0...v6.3.0) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: 6.3.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* docs(helm): Add recommended post-installation configuration guide Signed-off-by: John Duncan <30937261+roboav8r@users.noreply.github.com> Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com>
chore: update startup integration tests [AS-1463]
fix: detect gpu metrics in k8 gpu delegated-executor sidecars
…v2.21.0 api:v2.21.0 app:v2.21.0 cas:v2.21.0
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/helm/delegated-operator-instance-deployment_test.go (1)
5783-5895: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid new coverage; minor const-reuse nit.
Logic and assertions are correct — proper nil guards before dereferencing
sidecar, correct use of the sharedfindContainer/findVolume/findVolumeMounthelpers, and GPU key escaping matches existing conventions in this file.Line 5798/5803/5872 use the literal
"telemetry-sidecar"string instead of the package-leveltelemetrySidecarNameconst already defined intelemetry-sidecar_test.go(same package). Reusing the const avoids drift if the sidecar name ever changes.🤖 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 5783 - 5895, The new telemetry tests are correct, but they duplicate the sidecar name literal instead of reusing the shared telemetrySidecarName constant. Update TestTelemetryDisabledOmitsSidecar and TestTelemetrySidecarGpuEnv to reference telemetrySidecarName with the existing findContainer assertions so the name stays consistent if it changes elsewhere in the package.tests/unit/helm/delegated-operator-job-configmap_test.go (1)
762-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCorrect; near-duplicate of the Deployment-file GPU test.
Assertions and guards are sound (native-sidecar InitContainer lookup, nil-map-safe resource checks, proper
Require().NotNilbefore touchingsidecar.Env).
TestTelemetrySidecarGpuEnvhere duplicates ~80 lines ofTestTelemetrySidecarGpuEnvindelegated-operator-instance-deployment_test.goalmost verbatim (only the render/lookup path differs: ConfigMap→Job/InitContainers vs Deployment/Containers). Extracting the shared table-driven case data and assertion logic into a helper parameterized by a "get sidecar" closure would reduce risk of the two suites drifting apart over time.🤖 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 762 - 874, The GPU-sidecar test logic in TestTelemetrySidecarGpuEnv is duplicated almost verbatim in the Deployment test suite, so factor the shared table-driven cases and assertions into a helper instead of maintaining two copies. Keep the job-specific lookup in this file by passing a “get sidecar” closure or similar from TestTelemetrySidecarGpuEnv, and reuse the same helper for the Deployment variant to avoid drift while preserving the current ConfigMap→Job→InitContainers flow here.
🤖 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 `@helm/docs/post-install-recommended-configuration.md`:
- Around line 34-45: The dedicated plugins example only sets
FIFTYONE_PLUGINS_DIR, but it still needs the shared PVC mounted so /opt/plugins
persists. Update the dedicated plugins configuration to include
pluginsSettings.volumeMounts and pluginsSettings.volumes, and add the matching
apiSettings mount/volume entries so the deployment uses the PVC-backed plugin
directory correctly.
---
Nitpick comments:
In `@tests/unit/helm/delegated-operator-instance-deployment_test.go`:
- Around line 5783-5895: The new telemetry tests are correct, but they duplicate
the sidecar name literal instead of reusing the shared telemetrySidecarName
constant. Update TestTelemetryDisabledOmitsSidecar and
TestTelemetrySidecarGpuEnv to reference telemetrySidecarName with the existing
findContainer assertions so the name stays consistent if it changes elsewhere in
the package.
In `@tests/unit/helm/delegated-operator-job-configmap_test.go`:
- Around line 762-874: The GPU-sidecar test logic in TestTelemetrySidecarGpuEnv
is duplicated almost verbatim in the Deployment test suite, so factor the shared
table-driven cases and assertions into a helper instead of maintaining two
copies. Keep the job-specific lookup in this file by passing a “get sidecar”
closure or similar from TestTelemetrySidecarGpuEnv, and reuse the same helper
for the Deployment variant to avoid drift while preserving the current
ConfigMap→Job→InitContainers flow here.
🪄 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: ec5fbd09-dab4-40b2-88a4-f2e59f4c681f
⛔ Files ignored due to path filters (24)
.github/workflows/pre-commit.ymlis excluded by!**/*.yml.github/workflows/test-docker-pulls.ymlis excluded by!**/*.yml.github/workflows/test-docker.ymlis excluded by!**/*.yml.github/workflows/test-helm.ymlis excluded by!**/*.yml.github/workflows/test-integration-helm.ymlis excluded by!**/*.yml.pre-commit-config.yamlis excluded by!**/*.yamldocker/common-services.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/internal-auth/compose.delegated-operators.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.gpu.yamlis excluded by!**/*.yamldocker/legacy-auth/compose.delegated-operators.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/Chart.yamlis excluded by!**/*.yamlhelm/fiftyone-teams-app/templates/api-deployment.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-role.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!**/*.yamlhelm/values.yamlis excluded by!**/*.yamlskaffold.yamlis excluded by!**/*.yamltests/fixtures/helm/integration_values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (29)
CONTRIBUTING.mdMakefiledocker/README.mddocker/docs/configuring-gpu-workloads.mddocker/docs/upgrading.mddocs/custom-plugins.mddocs/orchestrators/configuring-databricks-orchestrator.mddocs/orchestrators/configuring-kubernetes-orchestrator.mdhelm/README.mdhelm/docs/post-install-recommended-configuration.mdhelm/docs/upgrading.mdhelm/fiftyone-teams-app/README.mdhelm/fiftyone-teams-app/README.md.gotmplhelm/fiftyone-teams-app/templates/NOTES.txthelm/fiftyone-teams-app/templates/_telemetry.tpltests/fixtures/docker/integration_internal_auth.envtests/fixtures/docker/integration_legacy_auth.envtests/integration/compose/docker-compose-internal-auth_test.gotests/integration/compose/docker-compose-legacy-auth_test.gotests/integration/helm/helm-internal-auth_test.gotests/integration/helm/helm-legacy-auth_test.gotests/integration/helm/helm-topology-spread-constraint_test.gotests/unit/compose/docker-compose-internal-auth_test.gotests/unit/compose/docker-compose-legacy-auth_test.gotests/unit/helm/delegated-operator-instance-deployment_test.gotests/unit/helm/delegated-operator-job-configmap_test.gotests/unit/helm/k8s_helpers.gotests/unit/helm/telemetry-rolebinding_test.gotests/unit/helm/telemetry-sidecar_test.go
| Dedicated plugins and delegated operators share a common plugin directory | ||
| backed by a Kubernetes PersistentVolume (PV) and PersistentVolumeClaim (PVC). | ||
|
|
||
| If you do not already have shared storage configured, refer to | ||
| [Adding Shared Storage for FiftyOne Enterprise Plugins](./plugins-storage.md) | ||
| for steps on creating a PV and PVC. | ||
|
|
||
| The examples below assume: | ||
|
|
||
| - PVC name: `plugins-pvc` | ||
| - Plugin directory: `/opt/plugins` | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'pluginsSettings|FIFTYONE_PLUGINS_DIR|plugins-pvc|teams-plugins' helm/fiftyone-teams-appRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- values.yaml outline ---'
ast-grep outline helm/fiftyone-teams-app/values.yaml --view expanded | sed -n '1140,1365p'
echo '--- template search for pluginsSettings volume wiring ---'
rg -n -C4 'pluginsSettings\.(volumeMounts|volumes|enabled|service|podSecurityContext|fsGroup|persistentVolumeClaim|claimName|mountPath|subPath)|FIFTYONE_PLUGINS_DIR' helm/fiftyone-teams-app/templates helm/fiftyone-teams-app/values.yaml
echo '--- post-install doc relevant section ---'
sed -n '1,120p' helm/docs/post-install-recommended-configuration.mdRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 16150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'delegatedOperatorDeployments\.(template\.)?(volumeMounts|volumes|env|enabled)|FIFTYONE_PLUGINS_DIR' helm/fiftyone-teams-app/templates helm/fiftyone-teams-app/values.yaml helm/docsRepository: voxel51/fiftyone-teams-app-deploy
Length of output: 11937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba helm/docs/post-install-recommended-configuration.md | sed -n '30,90p'
echo '---'
nl -ba helm/docs/post-install-recommended-configuration.md | sed -n '108,132p'Repository: voxel51/fiftyone-teams-app-deploy
Length of output: 211
Add the PVC mount to the dedicated plugins example The snippet only sets FIFTYONE_PLUGINS_DIR; pluginsSettings.enabled creates the deployment, but shared storage still has to be mounted explicitly via pluginsSettings.volumeMounts and pluginsSettings.volumes (and the matching apiSettings mount) or /opt/plugins won’t persist.
🤖 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/docs/post-install-recommended-configuration.md` around lines 34 - 45,
The dedicated plugins example only sets FIFTYONE_PLUGINS_DIR, but it still needs
the shared PVC mounted so /opt/plugins persists. Update the dedicated plugins
configuration to include pluginsSettings.volumeMounts and
pluginsSettings.volumes, and add the matching apiSettings mount/volume entries
so the deployment uses the PVC-backed plugin directory correctly.
|
We'll look into the docker unit tests checks after release. Kacey ran them locally and they passed on one of her PRs. |
No description provided.