fix(operator): align Helm template imagePullSecrets with CRD schema#1368
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request resolves a schema mismatch between the Helm template and CRD definition for image pull secrets. It adds deprecated support for the singular Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deployments/kai-scheduler/crds/kai.scheduler_configs.yaml (1)
3501-3505: Correct schema addition for backward compatibility.The deprecated field is properly typed and placed. The schema aligns with the Go struct's JSON tag
imagesPullSecret.One minor note: the deprecation message says "Use ImagePullSecrets instead" (referencing the Go field name), but users interacting via
kubectl explainor YAML would need to use the actual JSON field nameadditionalImagePullSecrets. Consider updating to:description: |- Deprecated: ImagePullSecret defines a single container registry secret credential. Use additionalImagePullSecrets instead.This would make the migration path clearer for users working directly with YAML manifests.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/kai-scheduler/crds/kai.scheduler_configs.yaml` around lines 3501 - 3505, Update the deprecation message for the imagesPullSecret CRD field to reference the actual JSON/YAML field name users should migrate to: change the description text in the imagesPullSecret schema (field name imagesPullSecret) to say "Use additionalImagePullSecrets instead" so kubectl explain/YAML users see the correct migration target.pkg/operator/config/image_pull_secrets.go (1)
19-30: Consider centralizing legacy/new merge logic in one place.This merge+dedeup path now exists here and in
pkg/apis/kai/v1/global.godefaulting. Keeping one normalization source would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/config/image_pull_secrets.go` around lines 19 - 30, Centralize the merge+dedupe logic for ImagePullSecret by extracting the block that checks globalConfig.ImagePullSecret and appends to secretDeploymentObjs into a single helper (e.g., MergeImagePullSecret or NormalizeImagePullSecrets) that accepts the pointer/string and the []v1.LocalObjectReference and returns a deduplicated slice; replace the duplicate logic in pkg/operator/config/image_pull_secrets.go (symbols: globalConfig.ImagePullSecret, secretDeploymentObjs) and the logic in pkg/apis/kai/v1/global.go with calls to this new helper so both locations share the same normalization code and behavior; add/update tests for the helper to cover nil/empty, existing, and new-secret cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deployments/kai-scheduler/templates/kai-config.yaml`:
- Around line 43-46: The YAML loop rendering additionalImagePullSecrets may emit
non-string scalars for numeric-like values; update the template in the
additionalImagePullSecrets block (the {{- range .Values.global.imagePullSecrets
}} loop and its item rendering) to explicitly quote each item so the rendered
list items are strings (e.g. render the item as a quoted string like "{{ . }}"
instead of - {{ . }}).
---
Nitpick comments:
In `@deployments/kai-scheduler/crds/kai.scheduler_configs.yaml`:
- Around line 3501-3505: Update the deprecation message for the imagesPullSecret
CRD field to reference the actual JSON/YAML field name users should migrate to:
change the description text in the imagesPullSecret schema (field name
imagesPullSecret) to say "Use additionalImagePullSecrets instead" so kubectl
explain/YAML users see the correct migration target.
In `@pkg/operator/config/image_pull_secrets.go`:
- Around line 19-30: Centralize the merge+dedupe logic for ImagePullSecret by
extracting the block that checks globalConfig.ImagePullSecret and appends to
secretDeploymentObjs into a single helper (e.g., MergeImagePullSecret or
NormalizeImagePullSecrets) that accepts the pointer/string and the
[]v1.LocalObjectReference and returns a deduplicated slice; replace the
duplicate logic in pkg/operator/config/image_pull_secrets.go (symbols:
globalConfig.ImagePullSecret, secretDeploymentObjs) and the logic in
pkg/apis/kai/v1/global.go with calls to this new helper so both locations share
the same normalization code and behavior; add/update tests for the helper to
cover nil/empty, existing, and new-secret cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d612cd37-8871-41b3-8fb9-c1c3d6b95555
📒 Files selected for processing (8)
CHANGELOG.mddeployments/kai-scheduler/crds/kai.scheduler_configs.yamldeployments/kai-scheduler/templates/kai-config.yamldeployments/kai-scheduler/tests/image_pull_secrets_test.yamlpkg/apis/kai/v1/global.gopkg/apis/kai/v1/zz_generated.deepcopy.gopkg/operator/config/image_pull_secrets.gopkg/operator/config/image_pull_secrets_test.go
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
@mfchanou The DCO check requires you to "sign off" on the commit |
mfchanou
force-pushed
the
fix/942-images-pull-secret-crd-mismatch
branch
from
b4006df to
a1fffe8
Compare
|
@enoodle done. feel free to have a look |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Update Helm template to write additionalImagePullSecrets (array) instead of the non-existent imagesPullSecret (string) field. Add deprecated imagesPullSecret field to CRD for backward compatibility with existing Config resources. Fixes kai-scheduler#942 Signed-off-by: mfchanou <chanoumahfuz@gmail.com>
mfchanou
force-pushed
the
fix/942-images-pull-secret-crd-mismatch
branch
from
310945c to
912f7c6
Compare
enoodle
added
backport v0.9
backport v0.6
backport v0.12
backport v0.13
backport v0.14
|
@enoodle any way to relaunch the failing job? seems to be an issue with the ci infra
|
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
Hi @enoodle I still need 2 approvers before merging the PR? could someone have a look ? |
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
@SiorMeir could you please approve this PR ? |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
Successfully created backport PR for |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin v0.6
git worktree add -d .worktree/backport-1368-to-v0.6 origin/v0.6
cd .worktree/backport-1368-to-v0.6
git switch --create backport-1368-to-v0.6
git cherry-pick -x 1499166060df0027abdffd7c6e27f3557e3d8d2b |
|
Successfully created backport PR for |
|
Successfully created backport PR for |
|
Successfully created backport PR for |
…1368) Signed-off-by: mfchanou <chanoumahfuz@gmail.com> Co-authored-by: Erez Freiberger <enoodle@gmail.com> Signed-off-by: Erez Freiberger <enoodle@gmail.com> Signed-off-by: mfchanou <38184317+mfchanou@users.noreply.github.com>
…1368) Signed-off-by: mfchanou <chanoumahfuz@gmail.com> Co-authored-by: Erez Freiberger <enoodle@gmail.com> Signed-off-by: Erez Freiberger <enoodle@gmail.com> Signed-off-by: mfchanou <38184317+mfchanou@users.noreply.github.com>
4 participants
Description
The Helm template for
kai-config.yamlwas writingspec.global.imagesPullSecret(a single string), but the CRD schema only definesspec.global.additionalImagePullSecrets(an array of strings). Kubernetes silently prunes unknown fields, so image pull secrets set via Helm were never applied.This PR:
additionalImagePullSecretsas an arrayimagesPullSecret(string) field to the CRD schemaRelated Issues
Fixes #942
Checklist
Breaking Changes
None. The deprecated
imagesPullSecretfield is preserved for backward compatibility. Existing Config CRs using this field will continue to work — the value is merged intoadditionalImagePullSecretsat runtime.Additional Notes
imagesPullSecretfield is marked as deprecated and should be removed in a future major versionglobal.imagePullSecrets[](previously only the first element was used)Summary by CodeRabbit
Bug Fixes
New Features
Chores