chore: Sync helm templates #16870
chore: Sync helm templates #16870
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications to Helm templates in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Datadog ReportAll test runs ✅ 68 Total Test Services: 0 Failed, 66 Passed Test ServicesThis report shows up to 10 services
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (22)
infra/helm/libs/job-template/templates/secrets.yaml (3)
1-2: Consider adding validation for secrets structureWhile the template checks for non-empty secrets, it doesn't validate the structure of the secrets map.
Consider adding validation for the secrets structure:
{{- if .Values.enabled }} -{{- if not (empty .Values.secrets) }} +{{- if and (not (empty .Values.secrets)) (kindIs "map" .Values.secrets) }}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
10-16: Review the systemManager backend configurationThe template hardcodes
systemManageras the backend type. This might limit reusability across different environments that use different secret backends.Consider making the backend type configurable through values:
spec: - backendType: systemManager + backendType: {{ .Values.secrets.backendType | default "systemManager" }}
1-18: Consider adding annotations for secret rotationTo enhance security, consider adding annotations to support secret rotation and lifecycle management.
Add support for refresh interval and other security annotations:
metadata: {{- if .Values.namespace }} namespace: {{ .Values.namespace }} {{- end }} name: {{ include "cronjob-template.fullname" . }} + annotations: + {{- if .Values.secrets.refreshInterval }} + refresh-interval: {{ .Values.secrets.refreshInterval | quote }} + {{- end }}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/serviceaccount.yaml (2)
7-9: Review namespace handling approachWhile the conditional namespace setting is implemented correctly, consider whether this ServiceAccount should always be created in a specific namespace for better isolation and security.
Consider:
- Documenting the implications of namespace configuration
- Adding validation for namespace value if it's required
- Implementing namespace isolation policies
1-17: Add documentation for required valuesThe template requires specific values to be set in values.yaml, but these requirements are not documented.
Add a comment block at the beginning of the file explaining:
{{/* Required values: - .Values.enabled: boolean to enable/disable the entire template - .Values.serviceAccount.create: boolean to control ServiceAccount creation - .Values.serviceAccount.annotations: map of annotations (optional) */}}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/pvc.yaml (2)
14-19: Enhance PVC specification flexibilityTwo improvements are needed for better Kubernetes compatibility:
- Support multiple access modes instead of a single string
- Make storageClassName optional as per Kubernetes spec
Apply this diff to enhance the template:
accessModes: - - {{ .accessModes }} + {{- toYaml .accessModes | nindent 2 }} resources: requests: storage: {{ .size }} - storageClassName: {{ .storageClass }} + {{- if .storageClass }} + storageClassName: {{ .storageClass }} + {{- end }}
1-22: Add template documentationConsider adding a comment block at the beginning of the template to document:
- The purpose of this template
- Required and optional values
- Example usage in values.yaml
Add documentation like this at the start of the file:
{{/* PersistentVolumeClaim template for job-template chart. Required values: - .Values.pvcs[].name: Name of the PVC - .Values.pvcs[].accessModes: List of access modes (e.g., ["ReadWriteOnce"]) - .Values.pvcs[].size: Storage size (e.g., "1Gi") Optional values: - .Values.pvcs[].storageClass: Storage class name - .Values.pvcs[].useExisting: Set to true to skip PVC creation Example values.yaml: pvcs: - name: data-volume accessModes: ["ReadWriteOnce"] size: 1Gi storageClass: standard */}}Note: The YAML syntax error reported by yamllint can be safely ignored as it's a false positive due to Helm templating.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/api-template/templates/pdb.yaml (3)
1-3: Consider adding namespace validationWhile the namespace variable is correctly defined, it's recommended to add validation to ensure the namespace value is provided in the Values.
Add this validation:
{{- $labels := include "api-template.labels" . -}} -{{- $namespace := $.Values.namespace -}} +{{- $namespace := required "A valid .Values.namespace entry required!" $.Values.namespace -}} {{- $serviceName := include "api-template.name" . -}}🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
4-11: Enhance template configuration and namingThe template structure is good, but could be improved in a few ways:
- The PDB suffix could be configurable
- The conditions could be combined for better readability
Consider this improvement:
-{{- if .Values.enabled }} -{{- if .Values.podDisruptionBudget }} +{{- if and .Values.enabled .Values.podDisruptionBudget }} apiVersion: policy/v1 kind: PodDisruptionBudget metadata: - name: {{ $serviceName }}-pdb + name: {{ $serviceName }}-{{ .Values.podDisruptionBudget.nameSuffix | default "pdb" }} namespace: {{ $namespace }}
1-22: Document PDB configuration in values.yamlTo ensure proper cluster availability, it's important to document the PDB configuration options and provide sensible defaults.
Add this to your values.yaml:
podDisruptionBudget: enabled: false # Enable PDB # Only one of minAvailable or maxUnavailable should be set minAvailable: 1 # Minimum number of pods that must be available # maxUnavailable: 1 # Maximum number of pods that can be unavailable nameSuffix: "pdb" # Suffix for PDB name🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/cronjob-template/values.yaml (2)
12-12: Consider documenting the startingDeadlineSeconds fieldThe
startingDeadlineSecondsfield is crucial for controlling how CronJobs handle missed or delayed schedules. When set tonull, there's no deadline, which might lead to unexpected behavior in production.Add a comment explaining the implications:
+# Deadline in seconds for starting the job if it misses scheduled time for any reason. +# Null means no deadline (the job will always be started). Set a value to ensure jobs don't +# stack up when the controller is down or during deployments. startingDeadlineSeconds: null
Line range hint
25-37: Security contexts should be explicitly definedThe security contexts are currently empty. For production workloads, it's recommended to explicitly define security settings.
Consider applying these security best practices:
podSecurityContext: - {} + fsGroup: 1000 + runAsNonRoot: true + runAsUser: 1000 + runAsGroup: 1000 securityContext: - {} + capabilities: + drop: + - ALL + readOnlyRootFilesystem: true + allowPrivilegeEscalation: falseinfra/helm/libs/job-template/values.yaml (1)
9-10: Add documentation for required value overrideThe repository value "defaultmissing" appears to be a placeholder. Consider:
- Adding a comment explaining this needs to be overridden
- Making it more obvious this is a required value
image: + # Required: Override this with your container image repository repository: defaultmissinginfra/helm/libs/api-template/templates/_helpers.tpl (1)
Line range hint
1-1: Document the new naming behavior in values.yaml.The new naming logic introduces additional flexibility, but it should be properly documented to guide users.
Please add documentation in the chart's
values.yamlfile explaining:
- The new
.Values.nameoption- The precedence order of name overrides (
.Values.name>.Values.nameOverride>.Chart.Name)- Examples of how different combinations of values affect the final name
infra/helm/libs/job-template/templates/_helpers.tpl (3)
5-7: Consider adding DNS subdomain name validationWhile the template follows Kubernetes naming length restrictions, consider adding validation to ensure the name complies with DNS subdomain name requirements (RFC 1123):
- Contains only lowercase alphanumeric characters or '-'
- Starts with an alphanumeric character
- Ends with an alphanumeric character
{{- define "job-template.name" -}} -{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }} +{{- $name := default .Chart.Name .Values.nameOverride | lower | trunc 63 | trimSuffix "-" }} +{{- if regexMatch "^[a-z0-9][a-z0-9-]*[a-z0-9]$" $name }} +{{- $name }} +{{- else }} +{{- fail (printf "Name %q must consist of lowercase alphanumeric characters or '-', start with an alphanumeric character, and end with an alphanumeric character" $name) }} +{{- end }} {{- end }}
30-32: Consider adding semantic version validationWhile the template handles basic version formatting, consider adding validation to ensure the chart version follows semantic versioning:
{{- define "job-template.chart" -}} -{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }} +{{- $version := .Chart.Version | replace "+" "_" }} +{{- if not (regexMatch "^v?(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$" $version) }} +{{- fail (printf "Chart version %q must be a valid semantic version" $version) }} +{{- end }} +{{- printf "%s-%s" .Chart.Name $version | trunc 63 | trimSuffix "-" }} {{- end }}
57-63: Consider adding service account name validationWhile the template handles the service account name generation, consider adding validation to ensure the name complies with Kubernetes naming requirements:
{{- define "job-template.serviceAccountName" -}} +{{- $name := "" }} {{- if .Values.serviceAccount.create }} -{{- default (include "job-template.fullname" .) .Values.serviceAccount.name }} +{{- $name = default (include "job-template.fullname" .) .Values.serviceAccount.name }} {{- else }} -{{- default "default" .Values.serviceAccount.name }} +{{- $name = default "default" .Values.serviceAccount.name }} +{{- end }} +{{- if not (regexMatch "^[a-z0-9][a-z0-9-]*[a-z0-9]$" $name) }} +{{- fail (printf "ServiceAccount name %q must consist of lowercase alphanumeric characters or '-', start with an alphanumeric character, and end with an alphanumeric character" $name) }} {{- end }} +{{- $name }} {{- end }}infra/helm/libs/job-template/templates/job.yaml (2)
66-101: Add validation for required environment variablesThe template accepts environment variables from multiple sources (global, local, secrets) but lacks validation for required variables. This could lead to silent failures if critical environment variables are missing.
Consider adding validation using Helm's
requiredfunction for critical environment variables:env: + {{- if not .Values.global.env.REQUIRED_VAR }} + {{- fail "REQUIRED_VAR must be set in global.env" }} + {{- end }} {{- range $key, $value := .Values.global.env }}Also, consider documenting the expected environment variables in the values.yaml file:
# values.yaml env: # Required variables REQUIRED_VAR: "" # Description of what this variable is for # Optional variables OPTIONAL_VAR: "" # Description of what this variable is for
132-132: Add validation for restart policy valuesThe restart policy defaults to "Never" but doesn't validate custom values. Kubernetes only allows specific values for job restart policies: "Never", "OnFailure".
Consider adding validation:
- restartPolicy: {{ .Values.restartPolicy | default "Never" }} + restartPolicy: {{ $policy := .Values.restartPolicy | default "Never" }} + {{- if not (or (eq $policy "Never") (eq $policy "OnFailure")) }} + {{- fail (printf "Invalid restartPolicy: %s. Must be either 'Never' or 'OnFailure'" $policy) }} + {{- end }} + {{ $policy }}infra/helm/libs/cronjob-template/templates/cronjob.yaml (2)
21-23: Consider adding validation for startingDeadlineSecondsWhile the conditional block for
startingDeadlineSecondsis correctly implemented, consider adding validation to ensure the value is positive and within reasonable bounds.{{- if .Values.startingDeadlineSeconds }} + {{- if and (kindIs "float64" .Values.startingDeadlineSeconds) (gt .Values.startingDeadlineSeconds 0) }} startingDeadlineSeconds: {{ .Values.startingDeadlineSeconds }} + {{- else }} + {{- fail "startingDeadlineSeconds must be a positive number" }} + {{- end }} {{- end }}
35-35: Consider making ttlSecondsAfterFinished optionalThe
ttlSecondsAfterFinishedfield is always set with a default value. Consider making it optional like other fields for more flexibility.- ttlSecondsAfterFinished: {{ .Values.ttlSecondsAfterFinished | default 600 }} + {{- if or .Values.ttlSecondsAfterFinished (eq (toString .Values.ttlSecondsAfterFinished) "0") }} + ttlSecondsAfterFinished: {{ .Values.ttlSecondsAfterFinished }} + {{- end }}infra/helm/libs/api-template/templates/deployment.yaml (1)
187-188: Review DataDog configuration implicationsA few considerations about the DataDog configuration changes:
- Setting
DD_DBM_PROPAGATION_MODE=fullenables full SQL query tracking, which might impact performance- Disabling profiling (
DD_PROFILING_ENABLED=false) while runtime metrics are enabled (DD_RUNTIME_METRICS_ENABLED=true) seems inconsistentConsider:
- Documenting the performance implications of full DBM propagation mode
- Evaluating if profiling should be enabled in alignment with runtime metrics
Also applies to: 197-198
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
infra/helm/libs/api-template/templates/_helpers.tpl(2 hunks)infra/helm/libs/api-template/templates/deployment.yaml(4 hunks)infra/helm/libs/api-template/templates/pdb.yaml(1 hunks)infra/helm/libs/cronjob-template/templates/cronjob.yaml(1 hunks)infra/helm/libs/cronjob-template/values.yaml(1 hunks)infra/helm/libs/job-template/.helmignore(1 hunks)infra/helm/libs/job-template/Chart.yaml(1 hunks)infra/helm/libs/job-template/templates/_helpers.tpl(1 hunks)infra/helm/libs/job-template/templates/job.yaml(1 hunks)infra/helm/libs/job-template/templates/pvc.yaml(1 hunks)infra/helm/libs/job-template/templates/secrets.yaml(1 hunks)infra/helm/libs/job-template/templates/serviceaccount.yaml(1 hunks)infra/helm/libs/job-template/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- infra/helm/libs/job-template/.helmignore
- infra/helm/libs/job-template/Chart.yaml
🧰 Additional context used
🪛 yamllint
infra/helm/libs/api-template/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/job.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/pvc.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/secrets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
infra/helm/libs/job-template/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (11)
infra/helm/libs/job-template/templates/secrets.yaml (1)
3-9: Verify ExternalSecret CRD compatibility
The template uses kubernetes-client.io/v1 API version. Please ensure this matches your cluster's External Secrets Operator version, as newer versions might use external-secrets.io/v1beta1.
infra/helm/libs/job-template/templates/serviceaccount.yaml (1)
11-11: Verify labels template consistency
The template is using cronjob-template.labels helper. Let's verify if this helper exists and is consistent across the templates.
✅ Verification successful
Let me gather more information about the label templates to verify their consistency.
Label templates are consistent and correctly used
The cronjob-template.labels helper is correctly defined in the cronjob-template's _helpers.tpl and follows the same structure as other templates (job-template and api-template). All templates include:
- helm.sh/chart label
- selector labels
- app version label
- managed-by label
The usage in serviceaccount.yaml is consistent with other resources in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for label helper template definitions and usage
# Search for label helper template definitions
echo "=== Searching for label helper template definitions ==="
rg -l "_helpers.tpl" infra/helm/libs/
for helper in $(rg -l "_helpers.tpl" infra/helm/libs/); do
echo "\n=== Content of $helper ==="
cat "$helper" | grep -A 5 "labels"
done
# Search for label helper usage
echo "\n=== Searching for label helper usage ==="
rg "template.labels" infra/helm/libs/
Length of output: 2360
Script:
#!/bin/bash
# Check the content of label helper definitions
echo "=== Content of job-template labels ==="
cat infra/helm/libs/job-template/templates/_helpers.tpl | grep -A 10 "job-template.labels"
echo -e "\n=== Content of cronjob-template labels ==="
cat infra/helm/libs/cronjob-template/templates/_helpers.tpl | grep -A 10 "cronjob-template.labels"
echo -e "\n=== Content of api-template labels ==="
cat infra/helm/libs/api-template/templates/_helpers.tpl | grep -A 10 "api-template.labels"
Length of output: 1531
infra/helm/libs/cronjob-template/values.yaml (1)
Line range hint 15-21: Review resource limits for production workload
The current resource configuration might need adjustment:
- CPU limit (400m) is 4x the request (100m), which could lead to throttling
- Memory limit (256Mi) is 2x the request (128Mi), which might be tight for some workloads
Let's analyze similar CronJobs in the cluster:
infra/helm/libs/job-template/values.yaml (1)
6-8: LGTM! Good security practices
Good defaults:
- Job disabled by default prevents accidental execution
- Dedicated service account follows the principle of least privilege
infra/helm/libs/api-template/templates/_helpers.tpl (1)
6-10: LGTM! Verify the name propagation in dependent templates.
The new naming logic is well-structured and maintains backward compatibility while adding flexibility through .Values.name. The implementation correctly handles Kubernetes naming requirements.
Let's verify how this new naming pattern is used across templates:
✅ Verification successful
The name template changes are safe and properly integrated
The new .Values.name override is well integrated across the template library:
- Used consistently in resource naming (configs, secrets, deployment, PDB)
- Properly referenced in labels and selectors
- Correctly propagated to the
fullnametemplate which maintains compatibility with the new naming pattern
The implementation maintains backward compatibility while adding the new override option, and all template usages are consistent with the changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of the name template across all Helm templates
# Expected: Find all template files that might need to be updated for the new naming pattern
# Search for uses of the name template
rg -t yaml -t tpl 'template.*"api-template\.name"' infra/helm/libs/
Length of output: 97
Script:
#!/bin/bash
# Search for uses of the name template without file type restrictions
rg 'template.*"api-template\.name"' infra/helm/libs/
# Also search for potential uses with single quotes
rg "template.*'api-template\.name'" infra/helm/libs/
# Search for any occurrences of api-template.name in case it's used differently
rg 'api-template\.name' infra/helm/libs/
# Let's also check the content of the entire _helpers.tpl file to understand the context better
cat infra/helm/libs/api-template/templates/_helpers.tpl
Length of output: 3408
infra/helm/libs/job-template/templates/_helpers.tpl (4)
1-4: LGTM! Proper file setup with vim modeline.
The vim filetype setting ensures proper syntax highlighting for template files.
14-25: LGTM! Well-implemented fullname template.
The template correctly implements the standard Helm chart naming pattern with proper handling of overrides and name composition.
37-44: LGTM! Follows Kubernetes labeling best practices.
The template implements all recommended Kubernetes labels as per kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/.
49-52: LGTM! Proper implementation of selector labels.
The template correctly implements the minimum required labels for Kubernetes selectors.
infra/helm/libs/job-template/templates/job.yaml (1)
55-65: Add validation for volume configurations
The template mounts volumes from PVCs and ConfigMaps without validating their existence or required configurations. This could lead to runtime failures if the volumes don't exist.
Let's verify the PVC configurations in the cluster:
Consider adding validation for volume configurations:
volumeMounts:
{{- if not (empty .Values.pvcs) }}
{{- range .Values.pvcs}}
+ {{- if not .name }}
+ {{- fail "PVC name is required" }}
+ {{- end }}
+ {{- if not .mountPath }}
+ {{- fail "mountPath is required for PVC" }}
+ {{- end }}
- mountPath: {{ .mountPath }}
name: {{ .name }}
{{- end}}
{{- end }}Also applies to: 117-131
infra/helm/libs/api-template/templates/deployment.yaml (1)
58-66: Verify ARM configuration compatibility with existing selectors
The ARM configuration looks correct, but we should verify it doesn't conflict with existing node selectors and tolerations defined in values.yaml.
✅ Verification successful
ARM configuration is compatible with existing node configurations
The implementation is safe because:
- The values.yaml shows empty defaults for
nodeSelector: {},tolerations: [], andaffinity: {} - The deployment.yaml correctly handles both ARM and non-ARM configurations:
- ARM settings are only applied when
.Values.armBetaEnrolledis true - Default node selectors and tolerations are applied separately using
.Values.nodeSelectorand.Values.tolerations - The template structure ensures no conflicts between ARM and custom configurations
- ARM settings are only applied when
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential conflicts in node selectors and tolerations
fd -e yaml -e yml values.yaml$ | while read -r file; do
echo "Checking $file for node configurations..."
yq e '.nodeSelector // .tolerations' "$file"
done
Length of output: 1367
Script:
#!/bin/bash
# Let's try a different approach using grep to check for node configurations
echo "Checking api-template values.yaml:"
cat infra/helm/libs/api-template/values.yaml
echo -e "\nChecking for existing nodeSelector or tolerations in deployment.yaml:"
cat infra/helm/libs/api-template/templates/deployment.yaml | grep -A 5 -B 5 'nodeSelector\|tolerations'
Length of output: 3205
| {{- if .Values.enabled }} | ||
| {{- range .Values.pvcs }} | ||
| {{- if not .useExisting }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required PVC configuration values
The template should validate that required PVC configuration values (name, accessModes, size, storageClass) are provided before attempting to create the resource.
Add validation like this at the start of the range loop:
{{- range .Values.pvcs }}
+{{- if not .name }}
+{{- fail "PVC name must be provided" }}
+{{- end }}
+{{- if not .accessModes }}
+{{- fail "PVC accessModes must be provided" }}
+{{- end }}
+{{- if not .size }}
+{{- fail "PVC size must be provided" }}
+{{- end }}
+{{- if not .storageClass }}
+{{- fail "PVC storageClass must be provided" }}
+{{- end }}
{{- if not .useExisting }}📝 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.
| {{- if .Values.enabled }} | |
| {{- range .Values.pvcs }} | |
| {{- if not .useExisting }} | |
| {{- if .Values.enabled }} | |
| {{- range .Values.pvcs }} | |
| {{- if not .name }} | |
| {{- fail "PVC name must be provided" }} | |
| {{- end }} | |
| {{- if not .accessModes }} | |
| {{- fail "PVC accessModes must be provided" }} | |
| {{- end }} | |
| {{- if not .size }} | |
| {{- fail "PVC size must be provided" }} | |
| {{- end }} | |
| {{- if not .storageClass }} | |
| {{- fail "PVC storageClass must be provided" }} | |
| {{- end }} | |
| {{- if not .useExisting }} |
| {{- if hasKey .Values.podDisruptionBudget "minAvailable" }} | ||
| minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} | ||
| {{- end }} | ||
| {{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }} | ||
| maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} | ||
| {{- end }} | ||
| selector: | ||
| matchLabels: | ||
| app.kubernetes.io/name: {{ $serviceName }} |
There was a problem hiding this comment.
Add validation for PDB configuration
The current implementation allows both minAvailable and maxUnavailable to be set simultaneously, which could lead to confusion. Additionally, the selector could be more flexible.
Consider these improvements:
spec:
{{- if hasKey .Values.podDisruptionBudget "minAvailable" }}
+{{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }}
+{{- fail "Cannot set both minAvailable and maxUnavailable in PDB" }}
+{{- end }}
minAvailable: {{ .Values.podDisruptionBudget.minAvailable }}
{{- end }}
{{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }}
maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }}
{{- end }}
selector:
matchLabels:
- app.kubernetes.io/name: {{ $serviceName }}
+ {{- include "api-template.selectorLabels" . | nindent 6 }}📝 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.
| {{- if hasKey .Values.podDisruptionBudget "minAvailable" }} | |
| minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} | |
| {{- end }} | |
| {{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }} | |
| maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} | |
| {{- end }} | |
| selector: | |
| matchLabels: | |
| app.kubernetes.io/name: {{ $serviceName }} | |
| {{- if hasKey .Values.podDisruptionBudget "minAvailable" }} | |
| {{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }} | |
| {{- fail "Cannot set both minAvailable and maxUnavailable in PDB" }} | |
| {{- end }} | |
| minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} | |
| {{- end }} | |
| {{- if hasKey .Values.podDisruptionBudget "maxUnavailable" }} | |
| maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} | |
| {{- end }} | |
| selector: | |
| matchLabels: | |
| {{- include "api-template.selectorLabels" . | nindent 6 }} |
| resources: | ||
| # We usually recommend not to specify default resources and to leave this as a conscious | ||
| # choice for the user. This also increases chances charts run on environments with little | ||
| # resources, such as Minikube. If you do want to specify resources, uncomment the following | ||
| # lines, adjust them as necessary, and remove the curly braces after 'resources:'. | ||
| limits: | ||
| cpu: 400m ## Research | ||
| memory: 256Mi | ||
| requests: | ||
| cpu: 100m | ||
| memory: 128Mi |
There was a problem hiding this comment.
💡 Codebase verification
Resource specifications need alignment with HPA configuration
The resource specifications appear inconsistent with the HPA configuration:
- CPU target utilization is set to 70% in HPA
- Current CPU limit (400m) and request (100m) seem arbitrary
- Same resource values are copy-pasted across job, cronjob, and api templates without workload consideration
- No documentation exists explaining the resource choices
Recommendations:
- Document the rationale for CPU/memory values
- Align CPU requests/limits with HPA's target utilization (70%)
- Consider different resource profiles for job/cronjob vs api workloads
- Remove the "Research" comment after proper investigation
🔗 Analysis chain
Review resource specifications and document rationale
- The "Research" comment suggests CPU limits need investigation
- Resource values seem arbitrary without context about the workload
- Consider documenting:
- Expected resource consumption patterns
- How these values were determined
- Recommendations for different workload sizes
Let me search for any documentation or configuration files that might provide context about these resource specifications.
Let me check the HPA (Horizontal Pod Autoscaler) configuration and any performance-related settings in the found files to understand if there's any correlation with the resource specifications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar resource specifications in other job templates
rg -A 5 "cpu: 400m" infra/helm/libs/
Length of output: 1111
Script:
#!/bin/bash
# Search for documentation about resource specifications
rg -i -g '*.{md,yaml,yml}' -B2 -A2 'resource|cpu|memory' 'infra/helm/libs/**/README*'
# Search for comments or documentation in template files
rg -i '(#|//).*resource' infra/helm/libs/
# Look for any performance or resource-related configuration
fd . infra/helm/libs/ -e yaml -e yml -x rg -l 'performance|workload|scaling'
Length of output: 1604
Script:
#!/bin/bash
# Check HPA and performance-related configurations
cat infra/helm/libs/api-template/templates/hpa.yaml
cat infra/helm/libs/api-template/values.debug.yaml
# Check if these resource values are referenced in any other configuration
rg -A 3 "400m|256Mi|100m|128Mi" infra/helm/libs/api-template/
Length of output: 3383
There was a problem hiding this comment.
we don´t use HPA, we use karpenter
There was a problem hiding this comment.
@busla, thank you for the clarification. I understand that you use Karpenter instead of HPA for scaling. Apologies for the confusion.
✏️ Learnings added
Learnt from: busla
PR: island-is/island.is#16870
File: infra/helm/libs/job-template/values.yaml:11-21
Timestamp: 2024-11-14T16:09:41.790Z
Learning: The project uses Karpenter instead of HPA for scaling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
incorrect, we are still using HPA
There was a problem hiding this comment.
@busla, thank you for correcting me. I now understand that you are still using HPA for scaling. Apologies for the misunderstanding.
✏️ Learnings added
Learnt from: busla
PR: island-is/island.is#16870
File: infra/helm/libs/job-template/values.yaml:11-21
Timestamp: 2024-11-14T16:10:22.874Z
Learning: The project uses HPA (Horizontal Pod Autoscaler) for scaling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| {{- $name := .Values.name | default .Chart.Name .Values.nameOverride -}} | ||
| {{- if contains $name .Release.Name -}} | ||
| {{- .Release.Name | trunc 63 | trimSuffix "-" -}} | ||
| {{- else -}} | ||
| {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
Fix the default value chain in name assignment.
There's a potential issue with the current implementation of the name assignment. The current pipe operation might not work as intended.
Apply this fix to correct the default value chain:
- {{- $name := .Values.name | default .Chart.Name .Values.nameOverride -}}
+ {{- $name := default (default .Chart.Name .Values.nameOverride) .Values.name -}}This ensures that:
- First, it evaluates
default .Chart.Name .Values.nameOverride - Then, it uses that as a fallback for
.Values.name
📝 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.
| {{- $name := .Values.name | default .Chart.Name .Values.nameOverride -}} | |
| {{- if contains $name .Release.Name -}} | |
| {{- .Release.Name | trunc 63 | trimSuffix "-" -}} | |
| {{- else -}} | |
| {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} | |
| {{- end -}} | |
| {{- $name := default (default .Chart.Name .Values.nameOverride) .Values.name -}} | |
| {{- if contains $name .Release.Name -}} | |
| {{- .Release.Name | trunc 63 | trimSuffix "-" -}} | |
| {{- else -}} | |
| {{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} | |
| {{- end -}} |
| ad.datadoghq.com/{{ .Chart.Name }}.logs: >- | ||
| [{ | ||
| "log_processing_rules": [{ | ||
| "type": "mask_sequences", | ||
| "name": "mask_national_ids", | ||
| "replace_placeholder": "--MASKED--", | ||
| "pattern" : "\\b(?:[89]\\d{3}|(?:[012]\\d|3[01])(?:0\\d|1[012]))\\d\\d-?\\d{4}\\b" | ||
| }] | ||
| }] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated national ID masking pattern
The national ID masking pattern is duplicated between Datadog logs configuration and APM configuration. This could lead to maintenance issues if the pattern needs to be updated.
Consider defining the pattern as a template helper or value:
+{{- define "job-template.maskingPattern" -}}
+\b(?:[89]\d{3}|(?:[012]\d|3[01])(?:0\d|1[012]))\d\d-?\d{4}\b
+{{- end }}Then use it in both locations:
- "pattern" : "\\b(?:[89]\\d{3}|(?:[012]\\d|3[01])(?:0\\d|1[012]))\\d\\d-?\\d{4}\\b"
+ "pattern": "{{ include "job-template.maskingPattern" . }}"And:
- "pattern": "\\b(?:[89]\\d{3}|(?:[012]\\d|3[01])(?:0\\d|1[012]))\\d\\d-?\\d{4}\\b",
+ "pattern": "{{ include "job-template.maskingPattern" . }}",Also applies to: 103-108
| apiVersion: batch/v1 | ||
| kind: Job | ||
| metadata: | ||
| name: "{{ .Chart.Name }}-{{ now | unixEpoch }}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more deterministic job naming strategy
Using now | unixEpoch in the job name will create a new job on every helm upgrade, even when the job specification hasn't changed. This could lead to:
- Unnecessary job proliferation
- Difficulty in tracking job history
- Resource accumulation
Consider using a more deterministic naming pattern, such as:
- name: "{{ .Chart.Name }}-{{ now | unixEpoch }}"
+ name: "{{ .Chart.Name }}-{{ include "job-template.fullname" . }}"Or if you need to ensure unique runs, use a combination of deterministic name and timestamp:
- name: "{{ .Chart.Name }}-{{ now | unixEpoch }}"
+ name: "{{ .Chart.Name }}-{{ include "job-template.fullname" . }}-{{ now | unixEpoch }}"Committable suggestion skipped: line range outside the PR's diff.
| {{- if .Values.armBetaEnrolled }} | ||
| tolerations: | ||
| - key: "arm" | ||
| operator: "Equal" | ||
| value: "true" | ||
| effect: "NoSchedule" | ||
| nodeSelector: | ||
| nodetype: "arm" | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make ARM configuration values configurable
The ARM-specific configurations use hard-coded values for tolerations and nodeSelector. Consider making these configurable through values.yaml for better flexibility.
{{- if .Values.armBetaEnrolled }}
tolerations:
- - key: "arm"
- operator: "Equal"
- value: "true"
- effect: "NoSchedule"
+ - key: {{ .Values.arm.toleration.key | default "arm" }}
+ operator: {{ .Values.arm.toleration.operator | default "Equal" }}
+ value: {{ .Values.arm.toleration.value | default "true" }}
+ effect: {{ .Values.arm.toleration.effect | default "NoSchedule" }}
nodeSelector:
- nodetype: "arm"
+ {{ .Values.arm.nodeSelector.key | default "nodetype" }}: {{ .Values.arm.nodeSelector.value | default "arm" }}
{{- end }}📝 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.
| {{- if .Values.armBetaEnrolled }} | |
| tolerations: | |
| - key: "arm" | |
| operator: "Equal" | |
| value: "true" | |
| effect: "NoSchedule" | |
| nodeSelector: | |
| nodetype: "arm" | |
| {{- end }} | |
| {{- if .Values.armBetaEnrolled }} | |
| tolerations: | |
| - key: {{ .Values.arm.toleration.key | default "arm" }} | |
| operator: {{ .Values.arm.toleration.operator | default "Equal" }} | |
| value: {{ .Values.arm.toleration.value | default "true" }} | |
| effect: {{ .Values.arm.toleration.effect | default "NoSchedule" }} | |
| nodeSelector: | |
| {{ .Values.arm.nodeSelector.key | default "nodetype" }}: {{ .Values.arm.nodeSelector.value | default "arm" }} | |
| {{- end }} |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration Changes
values.yamlto provide more flexible configuration options, including resource limits and security contexts.