service chart #53
service chart #53
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Helm chart "lfx-service" with chart metadata, README, values schema, helper templates, and templates for Deployment, ServiceAccount, Service, HPA, PDB, HTTPRoute, Traefik Heimdall middlewares, Heimdall RuleSet, ExternalSecret + SecretStore (AWS), and NATS KeyValue buckets — all gated by values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User (helm)
participant H as Helm (template)
participant K as Kubernetes API
participant T as Traefik
participant HC as Heimdall
participant ESO as ExternalSecretsOperator
participant AWS as AWS Secrets Manager
participant NO as NATS Operator
participant P as Pod
U->>H: helm install/upgrade lfx-service (values)
H->>K: Render & apply manifests (Deployment, Service, SA, HPA, PDB, ...)
alt httpRoute.enabled
H->>K: Create HTTPRoute
K-->>T: HTTPRoute reconciled
T->>P: Route traffic -> Service
end
alt heimdall.enabled
H->>K: Create Traefik Middlewares & RuleSet
T->>HC: forwardAuth requests (with/without body)
HC-->>T: AuthN/AuthZ decision
end
alt externalSecretsOperator.enabled && global.awsRegion
H->>K: Create SecretStore + ExternalSecret
K-->>ESO: ExternalSecret observed
ESO->>AWS: Fetch secrets
ESO->>K: Create/Update Kubernetes Secret
K->>P: Pod mounts/injects Secret
end
alt nats.kvBuckets defined
H->>K: Create KeyValue CRs
K-->>NO: Operator reconciles KV buckets
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (11)
🧰 Additional context used🪛 YAMLlint (1.37.1)charts/lfx-service/templates/serviceaccount.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/nats-kv-buckets.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/heimdall-middleware.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/httproute.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/externalsecret.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/deployment.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/hpa.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/pdb.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/ruleset.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/secretstore.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) charts/lfx-service/templates/service.yaml[error] 3-3: syntax error: expected the node content, but found '-' (syntax) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a comprehensive Helm chart for deploying LFX Platform V2 microservices with a focus on flexibility, best practices, and production readiness. The chart provides a reusable foundation for consistent deployments across LFX services.
Key Changes
- Created a complete Helm chart structure with standardized templating and helper functions
- Implemented comprehensive configuration support for deployment, networking, authentication, and secrets management
- Added documentation and examples for chart usage and configuration options
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/lfx-service/Chart.yaml | Chart metadata and version definition |
| charts/lfx-service/README.md | Comprehensive documentation with configuration examples and usage guide |
| charts/lfx-service/values.yaml | Default configuration values with extensive comments and examples |
| charts/lfx-service/templates/_helpers.tpl | Helper templates for naming, labeling, and common functions |
| charts/lfx-service/templates/deployment.yaml | Main application deployment template |
| charts/lfx-service/templates/service.yaml | Kubernetes service template |
| charts/lfx-service/templates/serviceaccount.yaml | Service account creation template |
| charts/lfx-service/templates/httproute.yaml | Gateway API HTTPRoute for ingress |
| charts/lfx-service/templates/heimdall-middleware.yaml | Traefik middleware for authentication |
| charts/lfx-service/templates/ruleset.yaml | Heimdall authentication rules |
| charts/lfx-service/templates/externalsecret.yaml | External secrets configuration for AWS Secrets Manager |
| charts/lfx-service/templates/secretstore.yaml | Secret store configuration for external secrets |
| charts/lfx-service/templates/hpa.yaml | Horizontal Pod Autoscaler configuration |
| charts/lfx-service/templates/pdb.yaml | Pod Disruption Budget template |
| charts/lfx-service/templates/nats-kv-buckets.yaml | NATS KeyValue bucket creation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/lfx-service/templates/externalsecret.yaml (1)
167-170: Fix incorrect value path referenceLine 167 references
.Values.externalSecrets.secret.namebut based on the rest of the template and values structure, this should likely be.Values.externalSecrets.target.name.Apply this fix:
-{{- if .Values.externalSecrets.secret.name }} -{{- .Values.externalSecrets.secret.name }} +{{- if .Values.externalSecrets.target.name }} +{{- .Values.externalSecrets.target.name }}
🧹 Nitpick comments (10)
charts/lfx-service/Chart.yaml (1)
4-9: Optional chart metadata improvements.Consider adding kubeVersion, home, sources, keywords, and maintainers for better discoverability (e.g., Artifact Hub).
charts/lfx-service/values.yaml (2)
197-214: HPA defaults OK; guard against empty metrics.If users clear all target* values, the HPA template could render an empty metrics list (invalid). See hpa.yaml comment for a defensive guard.
1-259: YAML lint nits: trailing spaces and missing EOF newline.There are multiple trailing spaces and no newline at EOF (per YAMLlint). Clean up to keep CI/lint green.
charts/lfx-service/templates/secretstore.yaml (1)
21-21: Quote AWS region (minor).Unquoted us-east-1 is valid YAML, but quoting avoids accidental interpretation.
- region: {{ .Values.global.awsRegion }} + region: {{ .Values.global.awsRegion | quote }}charts/lfx-service/templates/service.yaml (1)
20-23: Targeting named port "web" requires matching container port name.Verify the Deployment defines containerPort with name: web. If not, either rename the container port or set targetPort: {{ .Values.deployment.port }}.
- targetPort: web + targetPort: web + # Alternatively, if the container port is unnamed: + # targetPort: {{ .Values.deployment.port }}charts/lfx-service/templates/httproute.yaml (1)
40-43: Be explicit in backendRefs (optional).Optionally include kind: Service (and omit namespace to use same-namespace default) for clarity.
backendRefs: - - name: {{ include "lfx-v2-service-base.fullname" . }} - port: {{ .Values.service.port }} + - kind: Service + name: {{ include "lfx-v2-service-base.fullname" . }} + port: {{ .Values.service.port }}charts/lfx-service/templates/deployment.yaml (2)
28-32: Potential duplicate annotation renderingThe template appears to use
lfx-v2-service-base.annotationshelper instead oflfx-v2-service-base.mergedAnnotationsfor pod template annotations. This might not include thecommonAnnotations, which could lead to inconsistent annotation behavior between the deployment and its pods.Consider using the merged annotations helper for consistency:
- {{- $annotations := include "lfx-v2-service-base.annotations" . }} + {{- $annotations := include "lfx-v2-service-base.mergedAnnotations" (dict "context" . "resourceAnnotations" .Values.deployment.podAnnotations) }}
59-68: Validate environment variable configurationThe environment variable handling correctly supports both direct values and
valueFromreferences. However, consider adding validation to ensure bothvalueandvalueFromare not specified simultaneously for the same environment variable.Add validation similar to the ExternalSecret template:
{{- if .Values.deployment.environment }} env: {{- range $name, $config := .Values.deployment.environment }} + {{- if and $config.value $config.valueFrom }} + {{- fail (printf "Error: Cannot specify both 'value' and 'valueFrom' for environment variable '%s'" $name) }} + {{- end }} - name: {{ $name }} {{- if $config.value }} value: {{ $config.value | quote }} {{- else if $config.valueFrom }} valueFrom: {{- toYaml $config.valueFrom | nindent 14 }} {{- end }} {{- end }} {{- end }}charts/lfx-service/README.md (1)
335-336: Inconsistent parameter documentationThe values reference mentions
serviceAccount.awsRoleArnparameter, but this doesn't align with the typical ServiceAccount annotations pattern where AWS role ARN is specified as an annotation, not a direct value field.Consider clarifying or removing this parameter from the documentation if it's not actually used in the templates, or update to reflect the actual annotation-based pattern.
charts/lfx-service/templates/_helpers.tpl (1)
78-91: Consider adding annotations helper for pod templatesThe merged annotations helper is well-designed, but there's no dedicated helper for pod template annotations (as noted in the deployment template review). This could lead to inconsistent annotation handling.
Would you like me to generate an additional helper specifically for pod annotations that could merge common, deployment-level, and pod-specific annotations?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
charts/lfx-service/Chart.yaml(1 hunks)charts/lfx-service/README.md(1 hunks)charts/lfx-service/templates/_helpers.tpl(1 hunks)charts/lfx-service/templates/deployment.yaml(1 hunks)charts/lfx-service/templates/externalsecret.yaml(1 hunks)charts/lfx-service/templates/heimdall-middleware.yaml(1 hunks)charts/lfx-service/templates/hpa.yaml(1 hunks)charts/lfx-service/templates/httproute.yaml(1 hunks)charts/lfx-service/templates/nats-kv-buckets.yaml(1 hunks)charts/lfx-service/templates/pdb.yaml(1 hunks)charts/lfx-service/templates/ruleset.yaml(1 hunks)charts/lfx-service/templates/secretstore.yaml(1 hunks)charts/lfx-service/templates/service.yaml(1 hunks)charts/lfx-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-service/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-service/templates/ruleset.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/httproute.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/nats-kv-buckets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/secretstore.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/values.yaml
[error] 33-33: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 53-53: trailing spaces
(trailing-spaces)
[error] 62-62: trailing spaces
(trailing-spaces)
[error] 67-67: trailing spaces
(trailing-spaces)
[error] 89-89: trailing spaces
(trailing-spaces)
[error] 131-131: trailing spaces
(trailing-spaces)
[error] 164-164: trailing spaces
(trailing-spaces)
[error] 218-218: trailing spaces
(trailing-spaces)
[error] 224-224: trailing spaces
(trailing-spaces)
[error] 230-230: trailing spaces
(trailing-spaces)
[error] 240-240: trailing spaces
(trailing-spaces)
[error] 252-252: trailing spaces
(trailing-spaces)
[error] 259-259: no new line character at the end of file
(new-line-at-end-of-file)
charts/lfx-service/templates/heimdall-middleware.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/deployment.yaml
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
🪛 GitHub Actions: License Header Check
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: License header missing in charts/lfx-service/templates/externalsecret.yaml. License header check failed (exit code 1).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (14)
charts/lfx-service/values.yaml (3)
23-28: image.tag default relies on Chart.appVersion.This is good, but only once Chart.yaml defines appVersion (currently missing). See suggested fix in Chart.yaml comment.
68-89: Probe defaults look sane; verify container exposes named port "web".Liveness/readiness/startup probes target port name "web". Ensure the Deployment defines a containerPort with name: web; otherwise these probes will fail.
94-103: Service port consistency.Service.port defaults to 8080. Confirm Deployment’s target containerPort name "web" maps to the same numeric port to avoid 503s.
charts/lfx-service/templates/secretstore.yaml (2)
1-1: Ignore YAMLlint error for Helm control line.The leading “{{- if …}}” confuses plain YAML linters; this is fine in Helm.
25-26: Ensure ServiceAccount exists when referenced.If serviceAccount.create=false and no explicit name is provided, confirm the helper lfx-v2-service-base.serviceAccountName resolves to an existing SA; otherwise External Secrets auth will fail.
charts/lfx-service/templates/service.yaml (1)
1-26: Template structure LGTM.Conditional rendering, merged labels/annotations, and selector usage look correct.
charts/lfx-service/templates/serviceaccount.yaml (1)
1-18: LGTM.Conditional creation, metadata merging, and automount setting are correct for v1 ServiceAccount.
charts/lfx-service/templates/ruleset.yaml (2)
1-35: Good use of helpers and quoting.Naming, label/annotation merging, and quoted IDs look consistent.
21-34: RuleSet match schema conforms to Heimdall v1alpha4 — no change required.match.methods (array) and match.routes with "- path: " are valid for v1alpha4; the execute pipeline shape is also correct.
File: charts/lfx-service/templates/ruleset.yaml (lines 21-34)charts/lfx-service/templates/httproute.yaml (1)
1-42: General structure LGTM.Gateway API v1 usage, parentRefs, and hostname defaulting via helper look good.
charts/lfx-service/templates/hpa.yaml (1)
1-23: Overall HPA template looks good.autoscaling/v2 API, target ref, and label/annotation merging are correct.
charts/lfx-service/templates/nats-kv-buckets.yaml (1)
1-46: LGTM! Well-structured NATS KV buckets templateThe template correctly implements conditional rendering based on
.Values.nats.kvBucketsand individual bucket creation flags. The use of helper templates for labels and annotations ensures consistency across resources.charts/lfx-service/templates/externalsecret.yaml (1)
1-71: Well-designed ExternalSecret template with proper validationThe template correctly validates mutually exclusive configurations (data vs dataFrom) and provides sensible defaults for tag-based discovery. The conditional rendering based on AWS region ensures proper integration with AWS Secrets Manager.
charts/lfx-service/templates/heimdall-middleware.yaml (1)
1-49: LGTM! Clear documentation and proper middleware configurationThe template provides two middleware variants with clear comments explaining when to use each. The conditional rendering and label/annotation handling are properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
charts/lfx-service/templates/hpa.yaml (1)
24-40: Guard metrics block to avoid emitting an empty list.If both CPU and memory targets are unset, the empty metrics array is invalid.
Apply this diff:
- metrics: - {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} + {{- if or .Values.autoscaling.targetCPUUtilizationPercentage .Values.autoscaling.targetMemoryUtilizationPercentage }} + metrics: + {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} - type: Resource resource: name: cpu target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} {{- end }} {{- if .Values.autoscaling.targetMemoryUtilizationPercentage }} - type: Resource resource: name: memory target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} {{- end }} + {{- end }}charts/lfx-service/Chart.yaml (1)
8-9: Add appVersion to avoid empty image tag defaults.image.tag defaults to Chart.AppVersion via helpers; without appVersion the rendered image may be empty and deployments fail.
Apply this diff:
type: application version: 1.0.0 +appVersion: "1.0.0" icon: https://github.com/linuxfoundation/lfx-v2-helm/raw/main/img/lfx-logo-color.svgcharts/lfx-service/templates/_helpers.tpl (1)
1-1: Remove stray leading character breaking template parsing.The leading "i" will leak into rendered manifests and break YAML.
Apply this diff:
-i{{/* +{{/*charts/lfx-service/templates/httproute.yaml (1)
29-35: Guard empty matches.Only render matches when provided; otherwise you emit an empty list which can be invalid. This mirrors the earlier feedback.
rules: - - matches: - {{- range .Values.httpRoute.matches }} - - path: - type: {{ .path.type }} - value: {{ .path.value }} - {{- end }} + - {{- if .Values.httpRoute.matches }} + matches: + {{- range .Values.httpRoute.matches }} + - path: + type: {{ .path.type }} + value: {{ .path.value }} + {{- end }} + {{- end }}charts/lfx-service/templates/pdb.yaml (1)
17-23: Validate mutually exclusive PDB fields.Disallow specifying both minAvailable and maxUnavailable.
spec: + {{- if and .Values.podDisruptionBudget.minAvailable .Values.podDisruptionBudget.maxUnavailable }} + {{- fail "Error: Cannot specify both 'minAvailable' and 'maxUnavailable' in podDisruptionBudget" }} + {{- end }} {{- if .Values.podDisruptionBudget.minAvailable }} minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} {{- end }} {{- if .Values.podDisruptionBudget.maxUnavailable }} maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} {{- end }}
🧹 Nitpick comments (8)
charts/lfx-service/templates/_helpers.tpl (1)
104-106: Guard default hostname when global.domain is unset.Rendering lfx-api. is invalid; provide a safe fallback.
Apply this diff:
{{- define "lfx-service.defaultHostname" -}} -{{- printf "lfx-api.%s" .Values.global.domain }} +{{- if .Values.global.domain }} +{{- printf "lfx-api.%s" .Values.global.domain }} +{{- else }} +{{- printf "%s.local" (include "lfx-service.name" .) }} +{{- end }} {{- end }}charts/lfx-service/templates/secretstore.yaml (1)
21-26: Ensure SA exists and is IRSA-enabled for AWS JWT auth.This references a ServiceAccount; make sure it’s created/selected and annotated with the role ARN in your values when using EKS IRSA.
Example values snippet (outside this file):
serviceAccount: create: true annotations: eks.amazonaws.com/role-arn: arn:aws:iam::<acct>:role/<role-name>charts/lfx-service/templates/serviceaccount.yaml (1)
10-16: Expose IRSA annotations through values (if needed).You already merge annotations; document in README that IRSA role ARN should be set here for AWS integrations.
charts/lfx-service/templates/nats-kv-buckets.yaml (1)
5-6: Nit: consider renaming 'creation' to 'create' for consistency.Elsewhere booleans are typically 'enabled'/'create'. Not blocking.
charts/lfx-service/templates/deployment.yaml (2)
34-36: Allow using an existing ServiceAccount.Currently serviceAccountName is only set when create=true; this blocks referencing an existing SA via serviceAccount.name.
- {{- if .Values.serviceAccount.create }} - serviceAccountName: {{ include "lfx-service.serviceAccountName" . }} - {{- end }} + {{- if or .Values.serviceAccount.create .Values.serviceAccount.name }} + serviceAccountName: {{ include "lfx-service.serviceAccountName" . }} + {{- end }}
57-67: Env var items without value/valueFrom render ambiguous entries.If neither value nor valueFrom is set, you produce “- name: FOO” only. Prefer defaulting to empty string for clarity and schema safety.
- name: {{ $name }} - {{- if $config.value }} - value: {{ $config.value | quote }} - {{- else if $config.valueFrom }} - valueFrom: - {{- toYaml $config.valueFrom | nindent 14 }} - {{- end }} + {{- if hasKey $config "valueFrom" }} + valueFrom: + {{- toYaml $config.valueFrom | nindent 14 }} + {{- else }} + value: {{ default "" $config.value | quote }} + {{- end }}charts/lfx-service/values.yaml (2)
118-119: forwardBody value is unused.The middleware template always creates both resources and doesn’t read heimdall.forwardBody. Either wire this to control which middleware is created or document it as unused.
202-202: Add newline at EOF.Minor formatting to satisfy linters/tools.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
charts/lfx-service/Chart.yaml(1 hunks)charts/lfx-service/README.md(1 hunks)charts/lfx-service/templates/_helpers.tpl(1 hunks)charts/lfx-service/templates/deployment.yaml(1 hunks)charts/lfx-service/templates/externalsecret.yaml(1 hunks)charts/lfx-service/templates/heimdall-middleware.yaml(1 hunks)charts/lfx-service/templates/hpa.yaml(1 hunks)charts/lfx-service/templates/httproute.yaml(1 hunks)charts/lfx-service/templates/nats-kv-buckets.yaml(1 hunks)charts/lfx-service/templates/pdb.yaml(1 hunks)charts/lfx-service/templates/ruleset.yaml(1 hunks)charts/lfx-service/templates/secretstore.yaml(1 hunks)charts/lfx-service/templates/service.yaml(1 hunks)charts/lfx-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-service/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/lfx-service/README.md
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-service/templates/nats-kv-buckets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/secretstore.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/httproute.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/deployment.yaml
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/values.yaml
[error] 202-202: no new line character at the end of file
(new-line-at-end-of-file)
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/heimdall-middleware.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/ruleset.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 GitHub Actions: License Header Check
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: charts/lfx-service/templates/externalsecret.yaml is missing the license header (license header check failed).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
🔇 Additional comments (5)
charts/lfx-service/templates/_helpers.tpl (1)
96-98: Image tag helper depends on Chart.AppVersion.With the Chart fix applied, this is fine. If you intentionally omit appVersion, ensure values.image.tag is always set.
charts/lfx-service/templates/service.yaml (1)
17-25: LGTM: service spec/selector wiring looks correct.charts/lfx-service/templates/nats-kv-buckets.yaml (1)
24-43: LGTM: KV spec fields and keep-policy annotation are correctly templated.charts/lfx-service/templates/ruleset.yaml (1)
5-6: No change required — apiVersion is current: heimdall.dadrus.github.com/v1alpha4.Verified against Heimdall docs (v0.15.0 & v0.16.4); leave as-is.
charts/lfx-service/templates/externalsecret.yaml (1)
64-70: ExternalSecret rewrite OK —mergeis supportedESO's dataFrom.rewrite supports both
mergeandregexp.mergeaccepts keys:strategy(Extract|JSON),into(required for JSON),conflictPolicy(Ignore|Error), andpriority. Replace the emptymerge: {}with explicit options only if a specific merge behavior is intended.
AlanSherman
force-pushed
the
asherman/service-chart
branch
2 times, most recently
from
bb910c0 to
49f4701
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
charts/lfx-service/Chart.yaml (1)
4-9: Add appVersion to match values.yaml defaulting.image.tag claims to default to Chart.appVersion, but appVersion isn’t set.
apiVersion: v2 name: lfx-service description: LFX Service v2 Helm chart type: application version: 1.0.0 +appVersion: "1.0.0" icon: https://github.com/linuxfoundation/lfx-v2-helm/raw/main/img/lfx-logo-color.svgcharts/lfx-service/templates/heimdall-middleware.yaml (1)
13-15: Prefix middleware names to avoid cross‑release collisions.Use the chart fullname.
- name: heimdall-forward-body + name: {{ include "lfx-service.fullname" . }}-heimdall-forward-body ... - name: heimdall + name: {{ include "lfx-service.fullname" . }}-heimdallAlso applies to: 35-37
charts/lfx-service/templates/hpa.yaml (1)
24-41: Avoid emitting an empty metrics list.Wrap metrics in an OR-guard so the list isn’t empty when both targets are unset.
- metrics: - {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} + {{- if or .Values.autoscaling.targetCPUUtilizationPercentage .Values.autoscaling.targetMemoryUtilizationPercentage }} + metrics: + {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} - type: Resource resource: name: cpu target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} {{- end }} {{- if .Values.autoscaling.targetMemoryUtilizationPercentage }} - type: Resource resource: name: memory target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} {{- end }} + {{- end }}charts/lfx-service/templates/deployment.yaml (1)
1-4: Fail fast if image.repository is missing.Avoid rendering an empty repo (":tag"); abort with a clear message.
# Copyright The Linux Foundation and each contributor to LFX. # SPDX-License-Identifier: MIT +{{- if not .Values.image.repository -}} +{{- fail "image.repository is required (set values.image.repository)" -}} +{{- end -}} ---charts/lfx-service/templates/pdb.yaml (1)
17-23: Disallow both minAvailable and maxUnavailable simultaneously.Kubernetes rejects PDBs specifying both; add a validation guard.
spec: + {{- if and .Values.podDisruptionBudget.minAvailable .Values.podDisruptionBudget.maxUnavailable }} + {{- fail "Error: Cannot specify both 'minAvailable' and 'maxUnavailable' in podDisruptionBudget" }} + {{- end }} {{- if .Values.podDisruptionBudget.minAvailable }} minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} {{- end }} {{- if .Values.podDisruptionBudget.maxUnavailable }} maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} {{- end }}charts/lfx-service/templates/externalsecret.yaml (1)
1-8: License header must be at top (CI failure).Move header above templating to satisfy the License Header Check.
-{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} -{{- /* Validate that both data and dataFrom are not specified */ -}} -{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} -{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} -{{- end }} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT ---- +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} +{{- /* Validate that both data and dataFrom are not specified */ -}} +{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} +{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} +{{- end }} +---charts/lfx-service/templates/httproute.yaml (1)
21-43: Fix YAML rendering: indentation, stray token, and conditional matches.Current output is invalid: hostnames list is mis-indented, filters block has a stray “ws”, and backendRefs list items are mis-indented. Also guard matches when empty.
hostnames: - {{- if .Values.httpRoute.hostnames }} - {{- range .Values.httpRoute.hostnames }} - - {{ . | quote }} - {{- end }} - {{- else }} - - {{ include "lfx-service.defaultHostname" . | quote }} - {{- end }} + {{- if .Values.httpRoute.hostnames }} + {{- toYaml .Values.httpRoute.hostnames | nindent 4 }} + {{- else }} + - {{ include "lfx-service.defaultHostname" . | quote }} + {{- end }} rules: - - matches: - {{- range .Values.httpRoute.matches }} - - path: - type: {{ .path.type }} - value: {{ .path.value }} - {{- end }} - {{- with .Values.httpRoute.filters }} - filters: - {{- toYaml . | nindent 4 }} ws - {{- end }} - backendRefs: - - name: {{ include "lfx-service.fullname" . }} - port: {{ .Values.service.port }} + - {{- if .Values.httpRoute.matches }} + matches: + {{- range .Values.httpRoute.matches }} + - path: + type: {{ .path.type }} + value: {{ .path.value }} + {{- end }} + {{- end }} + {{- with .Values.httpRoute.filters }} + filters: + {{- toYaml . | nindent 6 }} + {{- end }} + backendRefs: + - name: {{ include "lfx-service.fullname" . }} + port: {{ .Values.service.port }}
🧹 Nitpick comments (9)
charts/lfx-service/templates/heimdall-middleware.yaml (1)
27-27: Either wire .Values.heimdall.forwardBody or remove it.forwardBody is hardcoded true; the value key exists but is unused. Pick one:
- Wire it: set forwardBody: {{ .Values.heimdall.forwardBody }} and only render the alt middleware when not .Values.heimdall.forwardBody.
- Or remove heimdall.forwardBody from values.yaml.
charts/lfx-service/values.yaml (2)
156-165: Expose memory target to match HPA template.hpa.yaml reads .Values.autoscaling.targetMemoryUtilizationPercentage but it’s missing here.
autoscaling: enabled: false minReplicas: 1 maxReplicas: 10 targetCPUUtilizationPercentage: 80 + # Optional: add memory target to enable memory-based autoscaling + targetMemoryUtilizationPercentage: null
200-200: Add trailing newline.Minor formatting nit for tooling.
charts/lfx-service/templates/secretstore.yaml (1)
8-9: Allow overriding SecretStore name from values.Respect externalSecretsOperator.secretStore.name when set.
metadata: - name: {{ include "lfx-service.fullname" . }} + name: {{ default (include "lfx-service.fullname" .) .Values.externalSecretsOperator.secretStore.name }}charts/lfx-service/templates/nats-kv-buckets.yaml (1)
10-11: Name collision risk; consider release‑scoped names.Plain .name can collide across releases in the same namespace. Prefer prefixing with chart fullname, or add a flag to opt in.
- name: {{ .name }} + name: {{ printf "%s-%s" (include "lfx-service.fullname" $) .name }}If the CRD requires spec.bucket to equal metadata.name, mirror the same prefix there as well.
charts/lfx-service/templates/service.yaml (1)
1-3: Move license header above templating gate.Place the copyright/SPDX lines at the very top to satisfy header checks and keep consistency across templates.
-{{- if .Values.service.enabled -}} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if .Values.service.enabled -}}charts/lfx-service/templates/deployment.yaml (1)
34-36: Render serviceAccountName even when not creating the SA.Allow binding to an existing SA when create=false.
- {{- if .Values.serviceAccount.create }} - serviceAccountName: {{ include "lfx-service.serviceAccountName" . }} - {{- end }} + serviceAccountName: {{ include "lfx-service.serviceAccountName" . }}charts/lfx-service/templates/externalsecret.yaml (1)
22-22: Quote refreshInterval (duration string).Prevents accidental parsing issues and aligns with External Secrets’ duration expectations.
- refreshInterval: {{ .Values.externalSecretsOperator.externalSecret.refreshInterval }} + refreshInterval: {{ .Values.externalSecretsOperator.externalSecret.refreshInterval | quote }}charts/lfx-service/templates/pdb.yaml (1)
1-3: Standardize header placement across templates.Move header above the gating “if” for consistency with other files and to avoid header-check edge cases.
-{{- if .Values.podDisruptionBudget.enabled -}} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if .Values.podDisruptionBudget.enabled -}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
charts/lfx-service/Chart.yaml(1 hunks)charts/lfx-service/README.md(1 hunks)charts/lfx-service/templates/_helpers.tpl(1 hunks)charts/lfx-service/templates/deployment.yaml(1 hunks)charts/lfx-service/templates/externalsecret.yaml(1 hunks)charts/lfx-service/templates/heimdall-middleware.yaml(1 hunks)charts/lfx-service/templates/hpa.yaml(1 hunks)charts/lfx-service/templates/httproute.yaml(1 hunks)charts/lfx-service/templates/nats-kv-buckets.yaml(1 hunks)charts/lfx-service/templates/pdb.yaml(1 hunks)charts/lfx-service/templates/ruleset.yaml(1 hunks)charts/lfx-service/templates/secretstore.yaml(1 hunks)charts/lfx-service/templates/service.yaml(1 hunks)charts/lfx-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-service/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/lfx-service/templates/_helpers.tpl
- charts/lfx-service/README.md
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-service/templates/httproute.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/deployment.yaml
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/heimdall-middleware.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/nats-kv-buckets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/ruleset.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/secretstore.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/values.yaml
[error] 200-200: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: License Header Check
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: License header check failed: missing header in charts/lfx-service/templates/externalsecret.yaml.
🔇 Additional comments (1)
charts/lfx-service/values.yaml (1)
21-26: Ensure image.tag default actually works.This relies on Chart.appVersion. After adding appVersion, verify deployment templating falls back to .Chart.AppVersion when .Values.image.tag is empty.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
charts/lfx-service/templates/hpa.yaml (1)
24-40: Guard metrics block to avoid empty list.Emit metrics: only if at least one target is set.
- metrics: - {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} + {{- if or .Values.autoscaling.targetCPUUtilizationPercentage .Values.autoscaling.targetMemoryUtilizationPercentage }} + metrics: + {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} - type: Resource resource: name: cpu target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetCPUUtilizationPercentage }} {{- end }} {{- if .Values.autoscaling.targetMemoryUtilizationPercentage }} - type: Resource resource: name: memory target: type: Utilization averageUtilization: {{ .Values.autoscaling.targetMemoryUtilizationPercentage }} {{- end }} + {{- end }}charts/lfx-service/templates/deployment.yaml (1)
49-53: Require image.repository and default tag to Chart.appVersion.Current template allows empty repo/tag; enforce required repo and default tag.
--- +{{- $imageRepo := required "image.repository is required (set values.image.repository)" .Values.image.repository -}} +{{- $imageTag := default .Chart.AppVersion .Values.image.tag -}} apiVersion: apps/v1 @@ - - name: app - image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" + - name: app + image: "{{ $imageRepo }}:{{ $imageTag }}" imagePullPolicy: {{ .Values.image.pullPolicy }}charts/lfx-service/templates/heimdall-middleware.yaml (1)
13-15: Avoid name collisions; prefix with release fullname.Static names will collide across releases. Prefix with chart fullname.
- name: heimdall-forward-body + name: {{ include "lfx-service.fullname" . }}-heimdall-forward-body ... - name: heimdall + name: {{ include "lfx-service.fullname" . }}-heimdallAlso applies to: 35-37
charts/lfx-service/templates/httproute.yaml (2)
36-39: Remove stray token breaking YAML.
wsafter the toYaml block makes the rendered YAML invalid.{{- with .Values.httpRoute.filters }} filters: - {{- toYaml . | nindent 4 }} ws + {{- toYaml . | nindent 4 }} {{- end }}
29-35: Guard empty matches to avoid emitting an empty list.HTTPRoute.rules.matches is optional; emitting an empty list is invalid.
rules: - - matches: - {{- range .Values.httpRoute.matches }} - - path: - type: {{ .path.type }} - value: {{ .path.value }} - {{- end }} + - {{- if .Values.httpRoute.matches }} + matches: + {{- range .Values.httpRoute.matches }} + - path: + type: {{ .path.type }} + value: {{ .path.value }} + {{- end }} + {{- end }}charts/lfx-service/templates/externalsecret.yaml (1)
1-8: Move license header to top (fixes CI).Header must precede all templating.
-{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} -{{- /* Validate that both data and dataFrom are not specified */ -}} -{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} -{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} -{{- end }} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} +{{- /* Validate that both data and dataFrom are not specified */ -}} +{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} +{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} +{{- end }}charts/lfx-service/templates/pdb.yaml (1)
17-23: Validate mutually exclusive/required PDB fields.K8s requires exactly one of minAvailable or maxUnavailable. Add guards.
spec: + {{- if and .Values.podDisruptionBudget.minAvailable .Values.podDisruptionBudget.maxUnavailable }} + {{- fail "podDisruptionBudget: specify only one of 'minAvailable' or 'maxUnavailable'" }} + {{- end }} + {{- if not (or .Values.podDisruptionBudget.minAvailable .Values.podDisruptionBudget.maxUnavailable) }} + {{- fail "podDisruptionBudget: one of 'minAvailable' or 'maxUnavailable' is required" }} + {{- end }} {{- if .Values.podDisruptionBudget.minAvailable }} minAvailable: {{ .Values.podDisruptionBudget.minAvailable }} {{- end }} {{- if .Values.podDisruptionBudget.maxUnavailable }} maxUnavailable: {{ .Values.podDisruptionBudget.maxUnavailable }} {{- end }}
🧹 Nitpick comments (12)
charts/lfx-service/values.yaml (2)
200-200: Add trailing newline.YAMLlint flags missing newline at EOF. Add a final newline to satisfy linters.
167-200: Add values schema to enforce types and mutual exclusivity.Recommend a values.schema.json to validate fields (e.g., require image.repository, enforce oneOf for externalSecret.data vs dataFrom).
Example schema (place at charts/lfx-service/values.schema.json):
{ "$schema": "https://json-schema.org/draft-07/schema#", "type": "object", "properties": { "image": { "type": "object", "properties": { "repository": { "type": "string", "minLength": 1 }, "tag": { "type": "string" }, "pullPolicy": { "type": "string" } }, "required": ["repository"] }, "externalSecretsOperator": { "type": "object", "properties": { "externalSecret": { "type": "object", "oneOf": [ { "required": ["data"] }, { "required": ["dataFrom"] } ] } } } } }charts/lfx-service/templates/ruleset.yaml (1)
21-33: Avoid emitting empty match objects.If neither methods nor routes are set, emit no match block to satisfy strict CRD schemas.
- match: - {{- with .match.methods }} - methods: - {{- toYaml . | nindent 10 }} - {{- end }} - {{- with .match.routes }} - routes: - {{- range . }} - - path: {{ . }} - {{- end }} - {{- end }} + {{- $hasMethods := hasKey . "match" | and (hasKey .match "methods") }} + {{- $hasRoutes := hasKey . "match" | and (hasKey .match "routes") }} + {{- if or $hasMethods $hasRoutes }} + match: + {{- with .match.methods }} + methods: + {{- toYaml . | nindent 10 }} + {{- end }} + {{- with .match.routes }} + routes: + {{- range . }} + - path: {{ . }} + {{- end }} + {{- end }} + {{- end }}charts/lfx-service/templates/nats-kv-buckets.yaml (1)
5-5: Use conventional flag or support both create/creation.Field name creation is non-standard vs. create used elsewhere; support both to be user-friendly.
-{{- if .creation }} +{{- if or .create .creation }}charts/lfx-service/templates/deployment.yaml (2)
60-66: Env rendering drops falsy/empty values.Use hasKey checks so empty string, 0, or false are preserved; otherwise variables may be omitted.
- - name: {{ $name }} - {{- if $config.value }} - value: {{ $config.value | quote }} - {{- else if $config.valueFrom }} - valueFrom: - {{- toYaml $config.valueFrom | nindent 14 }} - {{- end }} + - name: {{ $name }} + {{- if hasKey $config "value" }} + value: {{ $config.value | quote }} + {{- else if hasKey $config "valueFrom" }} + valueFrom: + {{- toYaml $config.valueFrom | nindent 14 }} + {{- end }}
25-32: Consider adding imagePullSecrets support.Optional: allow users to set imagePullSecrets for private registries.
spec: + {{- with .Values.imagePullSecrets }} + imagePullSecrets: + {{- toYaml . | nindent 8 }} + {{- end }} {{- if .Values.serviceAccount.create }}charts/lfx-service/templates/httproute.yaml (1)
1-3: Place license header at top of file (before templating).To satisfy license checks, put the header before any Helm directives. Applies to all new templates for consistency.
-{{- if .Values.httpRoute.enabled -}} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if .Values.httpRoute.enabled -}}charts/lfx-service/README.md (2)
135-138: Fix invalid YAML in annotations example.Use
:not=, and quote the ARN.annotations: { - "eks.amazonaws.com/role-arn" = arn:aws:iam::1234567890:role/SERVICE-NAME + "eks.amazonaws.com/role-arn": "arn:aws:iam::1234567890:role/SERVICE-NAME" }
332-336: Align values reference keys with earlier sections.You document
app.*here, but earlier sections usedeployment.*. Unify to avoid confusion.charts/lfx-service/templates/pdb.yaml (1)
1-3: Place license header before templating.For consistency with license checks, put the header at top.
-{{- if .Values.podDisruptionBudget.enabled -}} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if .Values.podDisruptionBudget.enabled -}}charts/lfx-service/templates/heimdall-middleware.yaml (1)
1-3: Place license header before templating.Keep header as first lines.
-{{- if and .Values.heimdall.enabled .Values.heimdall.createMiddleware -}} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if and .Values.heimdall.enabled .Values.heimdall.createMiddleware -}}charts/lfx-service/templates/externalsecret.yaml (1)
22-23: Quote refreshInterval to preserve intended string.Prevents accidental parsing issues.
- refreshInterval: {{ .Values.externalSecretsOperator.externalSecret.refreshInterval }} + refreshInterval: {{ .Values.externalSecretsOperator.externalSecret.refreshInterval | quote }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
charts/lfx-service/Chart.yaml(1 hunks)charts/lfx-service/README.md(1 hunks)charts/lfx-service/templates/_helpers.tpl(1 hunks)charts/lfx-service/templates/deployment.yaml(1 hunks)charts/lfx-service/templates/externalsecret.yaml(1 hunks)charts/lfx-service/templates/heimdall-middleware.yaml(1 hunks)charts/lfx-service/templates/hpa.yaml(1 hunks)charts/lfx-service/templates/httproute.yaml(1 hunks)charts/lfx-service/templates/nats-kv-buckets.yaml(1 hunks)charts/lfx-service/templates/pdb.yaml(1 hunks)charts/lfx-service/templates/ruleset.yaml(1 hunks)charts/lfx-service/templates/secretstore.yaml(1 hunks)charts/lfx-service/templates/service.yaml(1 hunks)charts/lfx-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-service/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/lfx-service/Chart.yaml
- charts/lfx-service/templates/_helpers.tpl
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-service/templates/ruleset.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/nats-kv-buckets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/deployment.yaml
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/heimdall-middleware.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/httproute.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/secretstore.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/values.yaml
[error] 200-200: no new line character at the end of file
(new-line-at-end-of-file)
🪛 GitHub Actions: License Header Check
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: License header check failed. Missing license header in charts/lfx-service/templates/externalsecret.yaml.
🔇 Additional comments (5)
charts/lfx-service/values.yaml (1)
21-26: Documented tag default isn’t enforced by templates.values.yaml says tag defaults to Chart.appVersion, but deployment.yaml uses .Values.image.tag directly. Fix the template (see deployment.yaml comment).
charts/lfx-service/templates/serviceaccount.yaml (1)
5-18: LGTM.ServiceAccount template and optional annotations rendering look correct.
charts/lfx-service/templates/service.yaml (1)
17-26: LGTM.Service spec aligns with the deployment’s named port “web” and shared labels.
charts/lfx-service/README.md (1)
18-24: Verify install path in Quick Start.If the chart lives under charts/lfx-service, the command should likely reference that path.
charts/lfx-service/templates/externalsecret.yaml (1)
64-70: Confirmed — ESO supports dataFrom.rewrite withmerge. External Secrets Operator documents amergerewrite (example:rewrite: - merge: {}) for use underdataFrom.findand shows it merging secret keys as expected. (external-secrets.io)
Signed-off-by: Alan Sherman <[email protected]>
AlanSherman
force-pushed
the
asherman/service-chart
branch
from
1e70a11 to
3734499
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
charts/lfx-service/templates/ruleset.yaml (1)
1-1: Duplicate comment: Guard against empty rulescharts/lfx-service/templates/serviceaccount.yaml (1)
8-8: Duplicate comment: Honor serviceAccount.name overridesThe ServiceAccount name should use the
lfx-service.serviceAccountNamehelper instead of hardcoding the fullname, to properly handle user-supplied name overrides.charts/lfx-service/templates/httproute.yaml (1)
40-40: Duplicate comment: Remove stray "ws" tokenThere's a stray "ws" token after the toYaml output that will break the generated YAML.
Apply this diff to fix the issue:
- {{- toYaml . | nindent 4 }} ws + {{- toYaml . | nindent 4 }}charts/lfx-service/templates/deployment.yaml (1)
59-70: Duplicate comment: Environment variable validationThe environment variable rendering can produce invalid entries when neither value nor valueFrom is provided.
charts/lfx-service/templates/pdb.yaml (1)
18-23: Add validation for mutually exclusive PDB settings.The template allows both
minAvailableandmaxUnavailableto be set simultaneously, which is not valid for PodDisruptionBudget specifications.charts/lfx-service/templates/externalsecret.yaml (1)
6-7: License header placement issue.The license header is still placed after Helm template directives, causing CI to fail. The header must be at the very top of the file before any templating logic.
Apply this diff to move the license header to the top:
-{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} -{{- /* Validate that both data and dataFrom are not specified */ -}} -{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} -{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} -{{- end }} -# Copyright The Linux Foundation and each contributor to LFX. -# SPDX-License-Identifier: MIT +# Copyright The Linux Foundation and each contributor to LFX. +# SPDX-License-Identifier: MIT +{{- if and .Values.externalSecretsOperator.enabled .Values.global.awsRegion }} +{{- /* Validate that both data and dataFrom are not specified */ -}} +{{- if and .Values.externalSecretsOperator.externalSecret.data .Values.externalSecretsOperator.externalSecret.dataFrom }} +{{- fail "Error: Cannot specify both 'data' and 'dataFrom' in externalSecrets. Please use only one." }} +{{- end }}
🧹 Nitpick comments (2)
charts/lfx-service/templates/deployment.yaml (2)
34-38: Service account name handling could be improvedThe current logic has separate branches for serviceAccount.create vs deployment.serviceAccount.name, but this could lead to inconsistency when both are provided.
Consider using a single helper template to resolve the service account name consistently:
- {{- if .Values.serviceAccount.create }} - serviceAccountName: {{ .Values.serviceAccount.name | default (include "lfx-service.fullname" .) }} - {{- else if .Values.deployment.serviceAccount.name }} - serviceAccountName: {{ .Values.deployment.serviceAccount.name }} - {{- end }} + serviceAccountName: {{ include "lfx-service.serviceAccountName" . }}This would require defining the helper template in
_helpers.tplif not already present.
53-53: Consider validating image repositoryThe image reference could result in an invalid format if the repository is empty.
Consider adding validation for the required image.repository field:
+ {{- if not .Values.image.repository -}} + {{- fail "image.repository is required" -}} + {{- end }} image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
charts/lfx-service/Chart.yaml(1 hunks)charts/lfx-service/README.md(1 hunks)charts/lfx-service/templates/_helpers.tpl(1 hunks)charts/lfx-service/templates/deployment.yaml(1 hunks)charts/lfx-service/templates/externalsecret.yaml(1 hunks)charts/lfx-service/templates/heimdall-middleware.yaml(1 hunks)charts/lfx-service/templates/hpa.yaml(1 hunks)charts/lfx-service/templates/httproute.yaml(1 hunks)charts/lfx-service/templates/nats-kv-buckets.yaml(1 hunks)charts/lfx-service/templates/pdb.yaml(1 hunks)charts/lfx-service/templates/ruleset.yaml(1 hunks)charts/lfx-service/templates/secretstore.yaml(1 hunks)charts/lfx-service/templates/service.yaml(1 hunks)charts/lfx-service/templates/serviceaccount.yaml(1 hunks)charts/lfx-service/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- charts/lfx-service/Chart.yaml
- charts/lfx-service/README.md
- charts/lfx-service/values.yaml
- charts/lfx-service/templates/_helpers.tpl
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-service/templates/secretstore.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/heimdall-middleware.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/httproute.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/deployment.yaml
[error] 10-10: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/hpa.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/nats-kv-buckets.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/pdb.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/ruleset.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/service.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-service/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🪛 GitHub Actions: License Header Check
charts/lfx-service/templates/externalsecret.yaml
[error] 1-1: License header missing in file 'charts/lfx-service/templates/externalsecret.yaml'.
🔇 Additional comments (18)
charts/lfx-service/templates/ruleset.yaml (2)
18-34: LGTM - Well-structured RuleSet templateThe RuleSet template is correctly structured with appropriate guards for optional fields, proper use of helper templates, and follows Helm best practices for metadata management and rule iteration.
5-6: I’m scanning the repository for all RuleSet CRD definitions and their declared apiVersions. This will confirm whether v1alpha4 is still in use or if v1beta1 is the current version.charts/lfx-service/templates/serviceaccount.yaml (1)
1-18: LGTM - Well-structured ServiceAccount templateThe ServiceAccount template properly handles conditional creation, uses appropriate helper templates for labels and annotations, and correctly manages the automountServiceAccountToken setting.
charts/lfx-service/templates/hpa.yaml (2)
24-42: LGTM - Proper metrics guard implementationThe HPA template correctly implements the guard for the metrics section using an OR condition to ensure at least one metric is configured before rendering the metrics block. This prevents invalid empty metrics arrays.
1-23: LGTM - Well-structured HPA templateThe HorizontalPodAutoscaler template correctly uses helper templates for naming and metadata management, and properly references the target deployment.
charts/lfx-service/templates/secretstore.yaml (2)
25-25: LGTM - Service account reference with fallbackThe service account name resolution uses a proper fallback pattern that first checks for a user-configured service account name, then defaults to the fullname if not provided. This provides good flexibility for different deployment scenarios.
1-26: LGTM - Proper SecretStore configurationThe SecretStore template is well-structured with appropriate conditional guards, uses helper templates correctly, and provides a sensible configuration for AWS Secrets Manager integration with JWT authentication.
charts/lfx-service/templates/service.yaml (1)
1-26: LGTM - Standard Service templateThe Service template follows Helm best practices with conditional rendering, proper use of helper templates for metadata, and standard service configuration with appropriate port mapping.
charts/lfx-service/templates/nats-kv-buckets.yaml (2)
15-23: LGTM - Thoughtful annotation handlingThe annotation logic properly handles both merged annotations from values and the conditional Helm resource policy annotation. The OR condition ensures the annotations block is only rendered when needed.
1-46: LGTM - Well-structured NATS KV templateThe template properly handles iteration over bucket configurations, uses conditional rendering for optional fields, and correctly applies Helm helper templates for consistent labeling and annotation management.
charts/lfx-service/templates/httproute.yaml (2)
30-37: LGTM - Proper matches guard implementationThe matches section correctly uses a conditional guard to only render the matches block when .Values.httpRoute.matches is provided, preventing invalid empty matches arrays.
1-29: LGTM - Well-structured HTTPRoute templateThe HTTPRoute template properly handles Gateway API resources with appropriate conditional rendering, helper template usage, and flexible hostname configuration with sensible defaults.
Also applies to: 38-45
charts/lfx-service/templates/deployment.yaml (1)
1-33: LGTM - Comprehensive Deployment templateThe Deployment template is well-structured with proper use of helper templates, conditional rendering for optional features, and follows Helm best practices for metadata management and resource configuration.
Also applies to: 39-91
charts/lfx-service/templates/heimdall-middleware.yaml (1)
1-49: YAMLlint warning is a false positive.The YAMLlint error about syntax is a false positive. Helm template files commonly start with conditionals and YAMLlint doesn't understand Helm templating syntax. The template structure is correct.
charts/lfx-service/templates/pdb.yaml (1)
1-27: YAMLlint warning is a false positive.The YAMLlint error about syntax is a false positive. Helm template files commonly start with conditionals and YAMLlint doesn't understand Helm templating syntax. The template structure is correct.
charts/lfx-service/templates/externalsecret.yaml (3)
3-5: Excellent validation logic for mutually exclusive options.The validation that prevents both
dataanddataFromfrom being specified simultaneously is well-implemented and provides clear error messaging.
38-70: Well-structured data handling with good defaults.The conditional logic for handling different data configurations is well-designed:
- Supports custom
dataFromconfiguration- Supports explicit
datamappings with comprehensive remoteRef options- Provides sensible tag-based discovery as default fallback
The implementation correctly handles all external secrets patterns.
28-37: Clean optional field handling.The use of
withdirectives for optional target policies and template configuration follows Helm best practices and keeps the rendered YAML clean.
Signed-off-by: Alan Sherman <[email protected]>
There was a problem hiding this comment.
We probably also want to document how to convert to using this helm chart -- particularly any service that is configured to keep resources (nats kv buckets, typically), will have those annotated with the helm chart that installed them, and fail if that helm chart changes names.. probably won't in most cases but might be worth calling out & documenting how to fix.
| - authorizer: allow_all | ||
| {{- end }} | ||
| {{- else }} | ||
| - authorizer: allow_all |
There was a problem hiding this comment.
There are two allow_all statements here after the elses -- it is possible to get them together? That seems like a mistake.
| object: {{ .authorization.object | quote }} | ||
| {{- else }} | ||
| # When OpenFGA is disabled, allow all requests | ||
| # (Only meant for *local development* because OpenFGA should be enabled when deployed) |
There was a problem hiding this comment.
In the spirit of this comment, if we have an environment key somewhere it's probably worth doing an if and to make sure the env isn't production and this isn't set at the same time and failing if it is.
| tolerations: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| containers: |
There was a problem hiding this comment.
Do we have support for an initContainer in here? We use them at least for the project service
There was a problem hiding this comment.
How much of the configuration in this chart is needed by our apps vs future proofing/just in case? Seems like every value here is able to be changed via values. Are we expecting our apps to set e.g. specific secret creation/deletion policies, image pull policy, affinity, or tolerations?
With all the complexity of templating stuff I would argue we should remove all the niche options we aren't going to use currently and add stuff back in as needed. We're trying to set guardrails and opinionated defaults for this chart, and all the options available that charts could set seems to defeat that.
|
|
||
| ## Values Reference | ||
|
|
||
| | Parameter | Description | Default | |
| --set image.tag=v1.0.0 \ | ||
| --values my-service-values.yaml | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Maybe add a link in here to the Examples section at the bottom of the file?
| environment: | ||
| LOG_LEVEL: | ||
| value: info | ||
| ``` |
There was a problem hiding this comment.
I think there should also be examples here of configuring the extra LFX services (heimdall, nats, ...), and one with them disabled if we want this chart to be consumed by non-LFX apps like the intercom identity-cookie-helper
| nameOverride: "" | ||
| fullnameOverride: "" |
There was a problem hiding this comment.
What's the difference between these? That should be documented somewhere
| # Common labels to apply to all resources | ||
| commonLabels: {} | ||
|
|
||
| # Common annotations to apply to all resources | ||
| commonAnnotations: {} |
There was a problem hiding this comment.
If these are common to all services should these go under global?
| # SPDX-License-Identifier: MIT | ||
| --- | ||
| # Global configuration that can be overridden per service | ||
| global: |
There was a problem hiding this comment.
global is handled specially by helm when using subcharts. Since we're not using subcharts here, should we use a different name for this shared section, or is this use close enough to the subchart usage its OK to reuse the name?
| serviceAccount: | ||
| create: true |
There was a problem hiding this comment.
We're defaulting this to true for local dev I assume, are we documenting somewhere that this should be disabled for prod deployments?
| {{- with .Values.deployment.resources }} | ||
| resources: | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- end }} No newline at end of file |
There was a problem hiding this comment.
nit: no trailing newline (other files too)
This pull request introduces a new reusable Helm chart for deploying LFX Platform V2 microservices, following best practices and providing a consistent, extensible deployment pattern. The changes include the initial chart definition, templating for deployments and Kubernetes resources, and comprehensive documentation for configuration and usage. Key features include flexible environment variable handling, integration with authentication and secrets management systems, and support for autoscaling and resource policies.
Helm Chart Definition and Documentation
Chart.yamland a detailedREADME.mddescribing chart features, configuration options, and usage examples for deploying LFX services. [1] [2]Templating and Resource Generation
_helpers.tplfor standardized naming, label/annotation merging, and resource configuration, ensuring consistency across all chart resources.Deployment,ExternalSecret,Heimdall Middleware, andHorizontalPodAutoscalerto support flexible, production-ready deployments with integrated authentication, secrets management, and autoscaling. [1] [2] [3] [4]Secrets Management and Validation
Authentication and Middleware Integration
Autoscaling Support