Thusara nginx service service #2
Conversation
WalkthroughA new Helm chart for deploying an NGINX service on Kubernetes was introduced, including chart metadata, templates, and default values. Supporting configuration files and templates were added for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant NGINX Pod
User->>Helm: Install nginx-service chart
Helm->>Kubernetes: Deploy Deployment and Service manifests
Kubernetes->>NGINX Pod: Create pods with specified image and resources
Kubernetes->>User: Expose NGINX via Service
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
templates/environment.py (1)
10-15: 🛠️ Refactor suggestionDuplicate
'cert-manager'entry inflates render loop
'cert-manager'appears twice inTEMPLATE_NAMES. The script renders the same template twice, printing duplicate log lines and performing redundant I/O.- 'cert-manager', ... - 'cert-manager',Remove one of them (keeping the original order) or convert the collection to a
setbefore the loop to guarantee uniqueness.
🧹 Nitpick comments (5)
charts/nginx-service/Chart.yaml (1)
26-29: Lock dependency digest or minor version to avoid breaking upgrades
bitnami/nginxmoves quickly. Consider pinningversion: ~20.1(tilde-range) or a specific patch version and/or usingrepository: oci://…with an immutable digest to guarantee reproducible builds.environments/gamma/values.yaml (1)
68-77: Remove trailing whitespace and quote the tag
Line 69 has trailing spaces that strict YAML parsers will reject. Also quote thelatesttag for consistency:- image: + image: tag: "latest"🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 69-69: trailing spaces
(trailing-spaces)
charts/nginx-service/templates/deployment.yaml (2)
10-11: Consider a rolling update strategy
Recreatecauses downtime by killing all pods at once. For high availability, switch toRollingUpdatewith sensiblemaxSurge/maxUnavailablesettings.
42-43: MakeserviceAccountNameconfigurable
Hard-codingmain-service-accountreduces chart reusability. Introduce aserviceAccountNamekey invalues.yamland use{{ .Values.serviceAccountName }}here.charts/nginx-service/templates/_helpers.tpl (1)
33-35: MakeddTagsresilient & reuse the helper-generated service name
$.Values.global.environmentNameis assumed to be always set.
If the chart is rendered stand-alone (outside the umbrella chart that injects theglobalblock) the lookup will be<no value>and the render will abort.While touching the helper, you can also avoid hard-coding
"nginx-service"by reusing thenginx-service.namehelper, keeping things DRY whennameOverride/fullnameOverrideare used.-{{- define "nginx-service.ddTags" -}} -{{- printf "env:%s, service:%s" $.Values.global.environmentName "nginx-service" }} -{{- end }} +{{- define "nginx-service.ddTags" -}} +{{- $env := default "unknown" $.Values.global.environmentName -}} +{{- $service := include "nginx-service.name" . -}} +{{- printf "env:%s, service:%s" $env $service }} +{{- end }}This keeps template rendering failure-proof and honours chart overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
charts/nginx-service/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
charts/nginx-service/.helmignore(1 hunks)charts/nginx-service/Chart.yaml(1 hunks)charts/nginx-service/templates/_helpers.tpl(1 hunks)charts/nginx-service/templates/deployment.yaml(1 hunks)charts/nginx-service/templates/service.yaml(1 hunks)charts/nginx-service/values.yaml(1 hunks)environments/gamma/rendered/app-of-apps.yaml(1 hunks)environments/gamma/rendered/nginx-service.yaml(1 hunks)environments/gamma/values.yaml(2 hunks)templates/environment.py(1 hunks)templates/nginx-service.j2(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/nginx-service/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
[warning] 32-32: wrong indentation: expected 6 but found 8
(indentation)
[warning] 33-33: wrong indentation: expected 6 but found 8
(indentation)
[warning] 34-34: wrong indentation: expected 6 but found 8
(indentation)
[warning] 37-37: wrong indentation: expected 4 but found 6
(indentation)
environments/gamma/values.yaml
[error] 69-69: trailing spaces
(trailing-spaces)
charts/nginx-service/templates/service.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (7)
templates/environment.py (1)
23-25: Tuple update is correct – welcome nginx-serviceAddition looks good and aligns with the new template/Helm chart.
environments/gamma/rendered/nginx-service.yaml (1)
5-7: Avoid using the mutablelatestimage tag in production
latestis not reproducible and breaks rollback guarantees. Prefer a semver, git-sha, or OCI digest tag that matches the build artifact promoted to gamma.charts/nginx-service/.helmignore (1)
4-24: 👍 Comprehensive.helmignoreGood coverage of common noise files; will keep chart packages slim.
environments/gamma/values.yaml (1)
128-130: Newnginx-serviceentry appended correctly
Thenginx-serviceblock is properly added underappOfApps.applicationsand disabled by default as intended.environments/gamma/rendered/app-of-apps.yaml (1)
119-126: Auto-generated file—no manual review needed
This section is produced byenvironment.py; it matches the new chart and value files.charts/nginx-service/templates/service.yaml (1)
1-16: Service template is correctly defined
The Helm template follows best practices: naming via helpers, configurableservice.type, and named port alignment with the Deployment.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
charts/nginx-service/values.yaml (1)
1-83: Values file structure looks solid
Defaults and documentation cover all key parameters. Consumers will overrideimage.tagand resources per environment.
| resources: | ||
| {{ nginxService.resources | to_yaml | indent(2) }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Indentation & default-safety for resources block
indent(2)does not indent the first line, causinglimits:to be indented two spaces while its children are indented four. Valid YAML but untidy and inconsistent with other templates.- If
nginxService.resourcesis omitted, Jinja will raise an exception.
-resources:
- {{ nginxService.resources | to_yaml | indent(2) }}
+resources:
+ {{ nginxService.resources | default({}) | to_yaml | indent(2, true) }}indent(2, true) keeps all lines aligned, and default({}) prevents template explosions when the key is absent.
📝 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.
| resources: | |
| {{ nginxService.resources | to_yaml | indent(2) }} | |
| resources: | |
| {{ nginxService.resources | default({}) | to_yaml | indent(2, true) }} |
🤖 Prompt for AI Agents
In templates/nginx-service.j2 at lines 3-4, the resources block uses indent(2)
which does not indent the first line, causing inconsistent YAML indentation, and
it lacks a default value for nginxService.resources which can cause exceptions
if the key is missing. Fix this by changing the filter to indent(2, true) to
indent all lines consistently and wrap nginxService.resources with default({})
to provide a safe fallback when the key is absent.
| image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default fallback for empty image tag
If Values.image.tag is blank, the image reference becomes invalid. Apply a default filter to fall back on .Chart.AppVersion:
image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"🤖 Prompt for AI Agents
In charts/nginx-service/templates/deployment.yaml at lines 47-48, the image tag
is used directly from .Values.image.tag which can be empty, causing an invalid
image reference. Fix this by applying the default filter to .Values.image.tag so
that if it is empty, it falls back to .Chart.AppVersion. Update the image line
to use: {{ .Values.image.tag | default .Chart.AppVersion }} to ensure a valid
tag is always used.
| ad.datadoghq.com/app-registry-service.logs: '[{"service": "app-registry-service"}]' | ||
| ad.datadoghq.com/app-registry-service.checks: | | ||
| { | ||
| "openmetrics": { | ||
| "instances": [ | ||
| { | ||
| "prometheus_url": "http://%%host%%:8081/metrics", | ||
| "namespace": "app-registry-service", | ||
| "metrics": ["*"] | ||
| } | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Datadog annotations reference the wrong service
The monitoring annotations are copy-pasted from app-registry-service (port 8081) but this chart’s container exposes port 80 as nginx. Update keys and ports:
-ad.datadoghq.com/app-registry-service.logs: '[{"service": "app-registry-service"}]'
-ad.datadoghq.com/app-registry-service.checks: |
+ad.datadoghq.com/nginx-service.logs: '[{"service": "nginx-service"}]'
+ad.datadoghq.com/nginx-service.checks: |
{
"openmetrics": {
"instances": [
{
- "prometheus_url": "http://%%host%%:8081/metrics",
+ "prometheus_url": "http://%%host%%:80/metrics",
"namespace": "nginx-service",
"metrics": ["*"]
}
]
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In charts/nginx-service/templates/deployment.yaml between lines 17 and 29, the
Datadog annotations incorrectly reference the app-registry-service and port
8081. Update the annotation keys to reference nginx instead of
app-registry-service, and change the prometheus_url port from 8081 to 80 to
match the nginx container's exposed port.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
charts/nginx-service/templates/deployment.yaml (1)
47-47: Add default fallback for empty image tagIf
.Values.image.tagis blank, theimage:line will be invalid. Apply thedefault .Chart.AppVersionfilter to ensure a valid tag.
🧹 Nitpick comments (2)
charts/nginx-service/templates/deployment.yaml (2)
9-10: Consider making the update strategy configurableUsing
Recreatewill lead to full downtime on each deployment update. Consider switching to aRollingUpdatedefault or exposing the strategy as a.Values.updateStrategyso it can be tuned per environment.
43-51: Add resource limits and health probesTo improve stability and observability, define
resources.requests/limits, and add bothlivenessProbeandreadinessProbefor the nginx container.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/nginx-service/templates/deployment.yaml(1 hunks)charts/nginx-service/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- charts/nginx-service/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/nginx-service/templates/deployment.yaml
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
[warning] 32-32: wrong indentation: expected 6 but found 8
(indentation)
[warning] 33-33: wrong indentation: expected 6 but found 8
(indentation)
[warning] 34-34: wrong indentation: expected 6 but found 8
(indentation)
[warning] 37-37: wrong indentation: expected 4 but found 6
(indentation)
🔇 Additional comments (2)
charts/nginx-service/templates/deployment.yaml (2)
1-6: No issues found in metadata definitionsThe API version, kind, metadata name, and labels follow Helm best practices and integrate correctly with the chart helpers.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 6-6: syntax error: expected the node content, but found '-'
(syntax)
30-34: Pod labels injection looks correctThe use of
toYamlunderpodLabelswill properly merge any custom labels with the chart’s standard labels.🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 32-32: wrong indentation: expected 6 but found 8
(indentation)
[warning] 33-33: wrong indentation: expected 6 but found 8
(indentation)
[warning] 34-34: wrong indentation: expected 6 but found 8
(indentation)
| ad.datadoghq.com/nginx-service.logs: '[{"service": "nginx-service"}]' | ||
| ad.datadoghq.com/nginx-service.checks: | | ||
| { | ||
| "openmetrics": { | ||
| "instances": [ | ||
| { | ||
| "prometheus_url": "http://%%host%%:8081/metrics", |
There was a problem hiding this comment.
Fix Datadog metrics port to match container port
The prometheus_url is currently targeting port 8081, but the nginx container listens on port 80. This mismatch will break metric scraping—update it to use :80/metrics.
🤖 Prompt for AI Agents
In charts/nginx-service/templates/deployment.yaml between lines 17 and 23,
update the prometheus_url port from 8081 to 80 to match the nginx container's
listening port. Change the URL from "http://%%host%%:8081/metrics" to
"http://%%host%%:80/metrics" to ensure proper metric scraping by Datadog.
| {{- end }} | ||
| securityContext: | ||
| {{- toYaml .Values.podSecurityContext | nindent 8 }} | ||
| serviceAccountName: main-service-account |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize serviceAccountName for greater flexibility
Hardcoding main-service-account limits reuse. Consider pulling this from .Values.serviceAccountName with a sensible default.
🤖 Prompt for AI Agents
In charts/nginx-service/templates/deployment.yaml at line 42, the
serviceAccountName is hardcoded as "main-service-account", which reduces
flexibility. Modify this line to use a Helm template expression that references
.Values.serviceAccountName with a default fallback value, allowing users to
override the service account name via values.yaml while maintaining a sensible
default.
Summary by CodeRabbit