✨ DSF: Support Extra Volumes Injection for hub components - #118
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends two controller Helm chart deployments to accept user-provided extra volumes and volume mounts. A new configuration contract in ChangesHub Controller Volume Configuration
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/dynamic-scoring-controller.yaml (1)
395-412: ⚡ Quick winConsider omitting empty volumeMounts and volumes fields rather than rendering empty lists.
The current pattern always renders the
volumeMounts:andvolumes:keys, then either the list items or[]. While functionally correct, the more idiomatic Kubernetes pattern is to omit optional fields entirely when they have no values. This matches the refactoring suggestion for the addon-controller template.♻️ Refactor to omit fields when empty
- volumeMounts: {{- with .Values.hubSetting.extraVolumeMounts }} -{{ toYaml . | nindent 10 }} - {{- else }} - [] + volumeMounts: + {{- toYaml . | nindent 10 }} {{- end }} securityContext: runAsNonRoot: true seccompProfile: type: RuntimeDefault serviceAccountName: dynamic-scoring-framework-controller terminationGracePeriodSeconds: 10 - volumes: {{- with .Values.hubSetting.extraVolumes }} -{{ toYaml . | nindent 8 }} - {{- else }} - [] + volumes: + {{- toYaml . | nindent 8 }} {{- end }}Note: The YAMLlint syntax error on line 397 is a false positive—Helm templates are not valid YAML before rendering.
🤖 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 `@dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/dynamic-scoring-controller.yaml` around lines 395 - 412, Replace the current always-rendered "volumeMounts:" and "volumes:" keys with conditional blocks so the keys are emitted only when their arrays are non-empty: use {{- with .Values.hubSetting.extraVolumeMounts }} to emit the "volumeMounts:" key followed by the indented {{ toYaml . | nindent 10 }} and close the with, and similarly use {{- with .Values.hubSetting.extraVolumes }} to emit "volumes:" with {{ toYaml . | nindent 8 }}; reference the existing .Values.hubSetting.extraVolumeMounts, .Values.hubSetting.extraVolumes and the volumeMounts/volumes entries in the dynamic-scoring-controller.yaml template when making the change.dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/addon-controller.yaml (1)
26-37: ⚡ Quick winConsider omitting empty volumeMounts and volumes fields rather than rendering empty lists.
The current pattern always renders the
volumeMounts:andvolumes:keys, then either the list items or[]. While functionally correct, the more idiomatic Kubernetes pattern is to omit optional fields entirely when they have no values.♻️ Refactor to omit fields when empty
- volumeMounts: {{- with .Values.hubSetting.extraVolumeMounts }} -{{ toYaml . | nindent 10 }} - {{- else }} - [] + volumeMounts: + {{- toYaml . | nindent 10 }} {{- end }} - volumes: {{- with .Values.hubSetting.extraVolumes }} -{{ toYaml . | nindent 8 }} - {{- else }} - [] + volumes: + {{- toYaml . | nindent 8 }} {{- end }}Note: The YAMLlint syntax error on line 28 is a false positive—Helm templates are not valid YAML before rendering.
🤖 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 `@dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/addon-controller.yaml` around lines 26 - 37, The template currently always emits the volumeMounts: and volumes: keys and renders [] when .Values.hubSetting.extraVolumeMounts or .Values.hubSetting.extraVolumes are empty; change the template so the entire volumeMounts and volumes keys are only rendered when there are items—e.g., use a conditional around each key (check .Values.hubSetting.extraVolumeMounts and .Values.hubSetting.extraVolumes or use with that places the key inside the with block) so that volumeMounts and volumes are omitted entirely when empty; reference the .Values.hubSetting.extraVolumeMounts, .Values.hubSetting.extraVolumes, and the volumeMounts/volumes keys in your change.
🤖 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.
Nitpick comments:
In
`@dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/addon-controller.yaml`:
- Around line 26-37: The template currently always emits the volumeMounts: and
volumes: keys and renders [] when .Values.hubSetting.extraVolumeMounts or
.Values.hubSetting.extraVolumes are empty; change the template so the entire
volumeMounts and volumes keys are only rendered when there are items—e.g., use a
conditional around each key (check .Values.hubSetting.extraVolumeMounts and
.Values.hubSetting.extraVolumes or use with that places the key inside the with
block) so that volumeMounts and volumes are omitted entirely when empty;
reference the .Values.hubSetting.extraVolumeMounts,
.Values.hubSetting.extraVolumes, and the volumeMounts/volumes keys in your
change.
In
`@dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/dynamic-scoring-controller.yaml`:
- Around line 395-412: Replace the current always-rendered "volumeMounts:" and
"volumes:" keys with conditional blocks so the keys are emitted only when their
arrays are non-empty: use {{- with .Values.hubSetting.extraVolumeMounts }} to
emit the "volumeMounts:" key followed by the indented {{ toYaml . | nindent 10
}} and close the with, and similarly use {{- with
.Values.hubSetting.extraVolumes }} to emit "volumes:" with {{ toYaml . | nindent
8 }}; reference the existing .Values.hubSetting.extraVolumeMounts,
.Values.hubSetting.extraVolumes and the volumeMounts/volumes entries in the
dynamic-scoring-controller.yaml template when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66869e19-3c5f-4e84-85b2-75f12613d6e2
📒 Files selected for processing (3)
dynamic-scoring-framework/charts/dynamic-scoring-framework/templates/addon-controller.yamldynamic-scoring-framework/charts/dynamic-scoring-framework/templates/dynamic-scoring-controller.yamldynamic-scoring-framework/charts/dynamic-scoring-framework/values.yaml
mikeshng
left a comment
mikeshng
left a comment
There was a problem hiding this comment.
/approve
/lgtm
This is a reasonable change by providing additional optional values. Thank you for your contribution!
Signed-off-by: xinkechen-evernorth <186659632+xinkechen-evernorth@users.noreply.github.com>
e27284d to
018e9f9
Compare
|
FYI - force pushed the change to include DCO signoff |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikeshng, xinkechen-evernorth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4394450
into
open-cluster-management-io:main
2 participants
Summary
This PR seeks to modify the Dynamic Scoring Framework helm chart templating to allow specification of additional volumes with the motivating use case being to inject additional ConfigMaps and Secrets volumes to augment the controller's trust store.
While deploying an custom scorer per the example and exposing it over TLS via our cluster ingress, we found the controller was unable to communicate with the custom scorer with an error message
x509: certificate signed by unknown authoritydue to our TLS certificate being issued by our internal CA.This PR fixes this issue by allowing one to provide a custom configmap with a relevant PEM encoded certificates (or as applicable have said data be injected by automation and mounting the relevant data over
/etc/ssl/certsto allow the controller verify the TLS certificate.Example:
trusted-caConfigmap:values.yamlsnippet:Related issue(s)
N/A
Summary by CodeRabbit