[INFRA-317] Update Plane-EE Helm Chart to version v2.3.0#212
Open
pratapalakshmi wants to merge 4 commits intomasterfrom
Open
[INFRA-317] Update Plane-EE Helm Chart to version v2.3.0#212pratapalakshmi wants to merge 4 commits intomasterfrom
v2.3.0#212pratapalakshmi wants to merge 4 commits intomasterfrom
Conversation
* Bump application version to 2.3.0 in Chart.yaml * Introduce new API secrets configuration in values.yaml for external secrets management * Add support for EKS Pod Identity and related annotations in service-account.yaml * Enhance config-secrets templates to include AWS Secrets Manager ARNs and key mappings * Add NOTES.txt for guidance on Pod Identity and secret management configuration
Contributor
WalkthroughBump plane-enterprise chart to v2.3.0 and add conditional AWS Secrets Manager ARN wiring, IRSA and EKS Pod Identity options, ServiceAccount annotation merging, and new values/key-mapping fields used by multiple templates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant K8s as Kubernetes
participant AWS as "AWS Secrets Manager / IAM"
User->>Helm: helm install/upgrade (values include podIdentity/irsa/secret ARNs)
Helm->>K8s: render manifests (ServiceAccount, Secrets, ConfigMaps, NOTES)
alt IRSA configured
K8s->>AWS: pod/ServiceAccount annotation -> STS AssumeRole on pod
else Pod Identity enabled
User->>AWS: install pod-identity-agent (out-of-band)
K8s->>AWS: pod-identity-agent maps pod -> IAM role
end
AWS-->>K8s: secrets/credentials become available to pods (via role)
Helm-->>User: NOTES.txt instructs setup and how to inspect rendered env vars
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
charts/plane-enterprise/templates/NOTES.txt (1)
39-39: Minor: Hyphenate compound adjective."Secret ARN driven" should be hyphenated as "Secret-ARN-driven" when used as a compound adjective before "env wiring".
📝 Proposed fix
--- -## Secret ARN driven env wiring +## Secret-ARN-driven env wiring🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/plane-enterprise/templates/NOTES.txt` at line 39, Update the header "## Secret ARN driven env wiring" to hyphenate the compound adjective so it reads "## Secret-ARN-driven env wiring"; edit the NOTES.txt header line (the header string "Secret ARN driven env wiring") to the hyphenated form to correct grammar.charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml (1)
116-122: Inconsistent whitespace control in template directive.Line 122 uses
{{ end }}while other closing directives use{{- end }}. This inconsistency can cause extra whitespace in the rendered output.♻️ Proposed fix
RDS_DB_PORT_KEY: {{ .Values.env.pi_envs.rds_db_port_key | quote }} RDS_DB_USERNAME_KEY: {{ .Values.env.pi_envs.rds_db_username_key | quote }} - {{ end }} + {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml` around lines 116 - 122, The closing Helm template directive for the conditional block is using "{{ end }}" which is inconsistent with the other closing directives that use whitespace-trimming "{{- end }}"; update the closing directive for the "if" block that checks .Values.services.pi.apiSecrets.rds_secret_arn and the various .Values.env.pi_envs.* keys to use "{{- end }}" so whitespace control is consistent with the opening "{{- if ... }}" and the other template closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/plane-enterprise/templates/config-secrets/app-env.yaml`:
- Around line 109-114: The default RUNNER_BASE_URL fallback currently hardcodes
the service as "plane-node-runner", causing mismatches with release-scoped
services; update the template logic that sets RUNNER_BASE_URL (the block using
.Values.env.runner_base_url) so the fallback host follows the same release-name
convention as other services by using .Release.Name (e.g., "{{ .Release.Name
}}-node-runner") combined with .Release.Namespace and port 3000, preserving the
existing conditional that prefers .Values.env.runner_base_url when provided.
In `@charts/plane-enterprise/templates/service-account.yaml`:
- Around line 1-10: The merge order currently gives earlier maps precedence, so
.Values.serviceAccount.annotations or .Values.irsa.annotations can block
.Values.irsa.roleArn; fix by reversing the overlay order so IRSA wins: when
merging into $annotations ensure the overlay maps for .Values.irsa.annotations
and the role ARN dict (".Values.irsa.roleArn") are passed as the first argument
to merge (i.e., treat them as the overriding maps) or build the final
$annotations by starting from serviceAccount then overlaying irsa.annotations
then overlaying the roleArn dict so that the role ARN and irsa annotations have
highest precedence; update the merge calls referencing $annotations,
.Values.serviceAccount.annotations, .Values.irsa.annotations, and
.Values.irsa.roleArn accordingly.
In `@charts/plane-enterprise/values.yaml`:
- Around line 466-471: The apiSecrets block contains hardcoded ARNs with a
real-looking AWS account ID; update amazonmq_secret_arn, elasticache_secret_arn,
and rds_secret_arn to use non-sensitive placeholder/example values (e.g.,
arn:aws:secretsmanager:REGION:ACCOUNT_ID:secret:your-secret-name or empty
strings) instead of the specific account number 426043895157, and optionally add
a short comment directing users to replace them with their own Secrets Manager
ARNs; ensure you update the symbols amazonmq_secret_arn, elasticache_secret_arn,
and rds_secret_arn in values.yaml accordingly.
- Around line 391-397: The comment for apiSecrets incorrectly states values are
base64-encoded; ARNs (e.g., follower_rds_secret_arn, model_secret_arn,
amazonmq_secret_arn, rds_secret_arn under apiSecrets and the duplicate comment
for env.apiSecrets) are plain-text identifiers — update the comment to reflect
that these fields expect ARN strings (or plain text secret identifiers) rather
than base64-encoded values, and apply the same correction to the duplicate
comment near env.apiSecrets.
---
Nitpick comments:
In `@charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml`:
- Around line 116-122: The closing Helm template directive for the conditional
block is using "{{ end }}" which is inconsistent with the other closing
directives that use whitespace-trimming "{{- end }}"; update the closing
directive for the "if" block that checks
.Values.services.pi.apiSecrets.rds_secret_arn and the various
.Values.env.pi_envs.* keys to use "{{- end }}" so whitespace control is
consistent with the opening "{{- if ... }}" and the other template closures.
In `@charts/plane-enterprise/templates/NOTES.txt`:
- Line 39: Update the header "## Secret ARN driven env wiring" to hyphenate the
compound adjective so it reads "## Secret-ARN-driven env wiring"; edit the
NOTES.txt header line (the header string "Secret ARN driven env wiring") to the
hyphenated form to correct grammar.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 466c00a1-7e11-4df6-95ee-f268f60c7db1
📒 Files selected for processing (8)
charts/plane-enterprise/Chart.yamlcharts/plane-enterprise/templates/NOTES.txtcharts/plane-enterprise/templates/config-secrets/app-env.yamlcharts/plane-enterprise/templates/config-secrets/live-env.yamlcharts/plane-enterprise/templates/config-secrets/pi-api-env.yamlcharts/plane-enterprise/templates/config-secrets/silo.yamlcharts/plane-enterprise/templates/service-account.yamlcharts/plane-enterprise/values.yaml
Comment on lines
+109
to
+114
| # Runner base URL used by the automation runner integration. | ||
| {{- if .Values.env.runner_base_url }} | ||
| RUNNER_BASE_URL: {{ .Values.env.runner_base_url | quote }} | ||
| {{- else }} | ||
| RUNNER_BASE_URL: "http://plane-node-runner.{{ .Release.Namespace }}.svc.cluster.local:3000" | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if node-runner service is defined in this chart with a fixed or release-based name
rg -n "node-runner" --type yaml charts/plane-enterprise/Repository: makeplane/helm-charts
Length of output: 224
🏁 Script executed:
# Check service definitions and deployments to understand the naming pattern
fd "service" charts/plane-enterprise/templates/ -type f | head -20Repository: makeplane/helm-charts
Length of output: 235
🏁 Script executed:
# Look for other service URL patterns in app-env.yaml to compare naming conventions
rg -A 2 "Release.Name" charts/plane-enterprise/templates/config-secrets/app-env.yaml | head -40Repository: makeplane/helm-charts
Length of output: 1917
🏁 Script executed:
# Check if there are any node-runner related deployments or services
fd . charts/plane-enterprise/templates/ -type f -exec rg -l "runner" {} \;Repository: makeplane/helm-charts
Length of output: 235
🏁 Script executed:
# Check values.yaml for runner configuration and service definitions
head -100 charts/plane-enterprise/values.yaml | grep -A 10 -B 10 "runner\|Runner" || rg -C 5 "runner.*service\|node.runner" charts/plane-enterprise/values.yaml | head -50Repository: makeplane/helm-charts
Length of output: 47
🏁 Script executed:
# Check if node-runner has deployment templates in this chart
find charts/plane-enterprise/templates -type f -name "*runner*" -o -type f -name "*node*"Repository: makeplane/helm-charts
Length of output: 47
🏁 Script executed:
# Look for Chart.yaml or dependencies that might reference node-runner
rg -i "dependencies|node-runner" charts/plane-enterprise/Chart.yamlRepository: makeplane/helm-charts
Length of output: 47
🏁 Script executed:
# Check if there's a separate node-runner chart
ls -la charts/ | grep -i runnerRepository: makeplane/helm-charts
Length of output: 47
🏁 Script executed:
# Check the values.yaml file for runner-related configuration
grep -n "runner" charts/plane-enterprise/values.yamlRepository: makeplane/helm-charts
Length of output: 158
🏁 Script executed:
# Look at the complete context of RUNNER_BASE_URL in app-env.yaml
sed -n '100,120p' charts/plane-enterprise/templates/config-secrets/app-env.yamlRepository: makeplane/helm-charts
Length of output: 1126
🏁 Script executed:
# Check if there are any other references to "plane-node-runner" in the entire chart
rg "plane-node-runner" charts/plane-enterprise/Repository: makeplane/helm-charts
Length of output: 220
🏁 Script executed:
# Check if there's any documentation about plane-node-runner in README or docs
find charts/plane-enterprise -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs grep -l "node-runner\|runner" 2>/dev/nullRepository: makeplane/helm-charts
Length of output: 49
🏁 Script executed:
# Check the git history to see if there's context about this naming choice
git log --oneline -n 30 -- charts/plane-enterprise/templates/config-secrets/app-env.yaml 2>/dev/null | head -10Repository: makeplane/helm-charts
Length of output: 108
🏁 Script executed:
# Look at the entire values.yaml section about services to understand the architecture
sed -n '480,510p' charts/plane-enterprise/values.yamlRepository: makeplane/helm-charts
Length of output: 850
Hardcoded service name doesn't follow release-name convention.
The default RUNNER_BASE_URL uses plane-node-runner instead of {{ .Release.Name }}-node-runner, which is inconsistent with every other internal service URL in this template (redis, pgdb, rabbitmq, monitor, live). If the node-runner service is deployed as part of this chart, this will cause connection failures when users deploy with a custom release name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plane-enterprise/templates/config-secrets/app-env.yaml` around lines
109 - 114, The default RUNNER_BASE_URL fallback currently hardcodes the service
as "plane-node-runner", causing mismatches with release-scoped services; update
the template logic that sets RUNNER_BASE_URL (the block using
.Values.env.runner_base_url) so the fallback host follows the same release-name
convention as other services by using .Release.Name (e.g., "{{ .Release.Name
}}-node-runner") combined with .Release.Namespace and port 3000, preserving the
existing conditional that prefers .Values.env.runner_base_url when provided.
Comment on lines
+1
to
+10
| {{- $annotations := dict -}} | ||
| {{- with .Values.serviceAccount.annotations -}} | ||
| {{- $annotations = merge $annotations . -}} | ||
| {{- end -}} | ||
| {{- with .Values.irsa.annotations -}} | ||
| {{- $annotations = merge $annotations . -}} | ||
| {{- end -}} | ||
| {{- if .Values.irsa.roleArn -}} | ||
| {{- $annotations = merge $annotations (dict "eks.amazonaws.com/role-arn" .Values.irsa.roleArn) -}} | ||
| {{- end -}} |
Contributor
There was a problem hiding this comment.
Merge precedence may surprise users expecting irsa.roleArn to override.
Helm's merge function gives precedence to the first (leftmost) map, meaning:
- If
serviceAccount.annotationscontainseks.amazonaws.com/role-arn, it will NOT be overridden byirsa.annotationsorirsa.roleArn. - If
irsa.annotationscontainseks.amazonaws.com/role-arn, it will NOT be overridden byirsa.roleArn.
This contradicts the comment in values.yaml (line 432) stating "Additional/override IRSA annotations". Consider documenting this precedence or reversing the merge order so irsa.roleArn takes highest precedence.
♻️ Option: Reverse merge order so irsa.roleArn wins
{{- $annotations := dict -}}
-{{- with .Values.serviceAccount.annotations -}}
-{{- $annotations = merge $annotations . -}}
-{{- end -}}
-{{- with .Values.irsa.annotations -}}
-{{- $annotations = merge $annotations . -}}
-{{- end -}}
{{- if .Values.irsa.roleArn -}}
-{{- $annotations = merge $annotations (dict "eks.amazonaws.com/role-arn" .Values.irsa.roleArn) -}}
+{{- $annotations = merge $annotations (dict "eks.amazonaws.com/role-arn" .Values.irsa.roleArn) -}}
+{{- end -}}
+{{- with .Values.irsa.annotations -}}
+{{- $annotations = merge $annotations . -}}
+{{- end -}}
+{{- with .Values.serviceAccount.annotations -}}
+{{- $annotations = merge $annotations . -}}
{{- end -}}This makes irsa.roleArn highest priority, then irsa.annotations, then serviceAccount.annotations.
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plane-enterprise/templates/service-account.yaml` around lines 1 - 10,
The merge order currently gives earlier maps precedence, so
.Values.serviceAccount.annotations or .Values.irsa.annotations can block
.Values.irsa.roleArn; fix by reversing the overlay order so IRSA wins: when
merging into $annotations ensure the overlay maps for .Values.irsa.annotations
and the role ARN dict (".Values.irsa.roleArn") are passed as the first argument
to merge (i.e., treat them as the overriding maps) or build the final
$annotations by starting from serviceAccount then overlaying irsa.annotations
then overlaying the roleArn dict so that the role ARN and irsa annotations have
highest precedence; update the merge calls referencing $annotations,
.Values.serviceAccount.annotations, .Values.irsa.annotations, and
.Values.irsa.roleArn accordingly.
Comment on lines
+391
to
+397
| apiSecrets: | ||
| # Used by PI API to look up external secrets/credentials. | ||
| # Values are expected to be base64-encoded strings (as provided by the user). | ||
| follower_rds_secret_arn: '' | ||
| model_secret_arn: '' | ||
| amazonmq_secret_arn: '' | ||
| rds_secret_arn: '' |
Contributor
There was a problem hiding this comment.
Misleading comment: ARNs are not base64-encoded.
The comment states "Values are expected to be base64-encoded strings" but AWS Secrets Manager ARNs are plain text identifiers (e.g., arn:aws:secretsmanager:region:account-id:secret:name). This same misleading comment also appears at lines 467-468 for env.apiSecrets.
📝 Proposed fix
apiSecrets:
- # Used by PI API to look up external secrets/credentials.
- # Values are expected to be base64-encoded strings (as provided by the user).
+ # Used by PI API to look up external secrets/credentials.
+ # Provide the full AWS Secrets Manager ARN.
follower_rds_secret_arn: ''Apply the same fix at line 467-468.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plane-enterprise/values.yaml` around lines 391 - 397, The comment for
apiSecrets incorrectly states values are base64-encoded; ARNs (e.g.,
follower_rds_secret_arn, model_secret_arn, amazonmq_secret_arn, rds_secret_arn
under apiSecrets and the duplicate comment for env.apiSecrets) are plain-text
identifiers — update the comment to reflect that these fields expect ARN strings
(or plain text secret identifiers) rather than base64-encoded values, and apply
the same correction to the duplicate comment near env.apiSecrets.
…ecrets Manager ARNs and additional Redis configuration. Introduce pod identity role ARN handling in service-account.yaml and add Redis authentication token key in live-env.yaml and silo.yaml.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/plane-enterprise/templates/service-account.yaml (1)
1-17:⚠️ Potential issue | 🟠 Major
mergeprecedence prevents the intended IRSA/Pod Identity overridesLine 12 says Pod Identity role ARN should override, but with
mergethe existing$annotationskeys win. As written, values fromserviceAccount.annotations/irsa.annotationscan still block bothirsa.roleArn(Line 9) andpodIdentity.roleArn(Line 13), resulting in the wrong role annotation being rendered.In Helm templates (Sprig dict functions), what is key precedence behavior for `merge` vs `mergeOverwrite`, and which one should be used when later maps (e.g., role-arn overrides) must take priority?♻️ Suggested fix (use overwrite semantics for overlay steps)
{{- $annotations := dict -}} {{- with .Values.serviceAccount.annotations -}} -{{- $annotations = merge $annotations . -}} +{{- $annotations = mergeOverwrite $annotations . -}} {{- end -}} {{- with .Values.irsa.annotations -}} -{{- $annotations = merge $annotations . -}} +{{- $annotations = mergeOverwrite $annotations . -}} {{- end -}} {{- if .Values.irsa.roleArn -}} -{{- $annotations = merge $annotations (dict "eks.amazonaws.com/role-arn" .Values.irsa.roleArn) -}} +{{- $annotations = mergeOverwrite $annotations (dict "eks.amazonaws.com/role-arn" .Values.irsa.roleArn) -}} {{- end -}} {{- if and .Values.podIdentity.enabled .Values.podIdentity.roleArn -}} {{- /* Pod Identity role ARN should override any IRSA role-arn when both are set. */ -}} -{{- $annotations = merge $annotations (dict "eks.amazonaws.com/role-arn" .Values.podIdentity.roleArn) -}} +{{- $annotations = mergeOverwrite $annotations (dict "eks.amazonaws.com/role-arn" .Values.podIdentity.roleArn) -}} {{- end -}} {{- if .Values.podIdentity.annotations -}} -{{- $annotations = merge $annotations .Values.podIdentity.annotations -}} +{{- $annotations = mergeOverwrite $annotations .Values.podIdentity.annotations -}} {{- end -}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/plane-enterprise/templates/service-account.yaml` around lines 1 - 17, The template uses Sprig's merge which preserves existing keys, so later overlays like .Values.irsa.roleArn and .Values.podIdentity.roleArn can be blocked; change the overlay merges to use mergeOverwrite so later values win. Concretely, keep the initial $annotations := dict and the base merge of .Values.serviceAccount.annotations, but replace the merge calls that apply .Values.irsa.annotations, the dict("eks.amazonaws.com/role-arn", .Values.irsa.roleArn), the dict("eks.amazonaws.com/role-arn", .Values.podIdentity.roleArn), and .Values.podIdentity.annotations with mergeOverwrite so the IRSA and Pod Identity role ARNs and podIdentity annotations take precedence as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/plane-enterprise/README.md`:
- Line 160: The README text for the services.redis.local_setup description
incorrectly mentions “aws rds”; update the wording to reference Redis-specific
hosted services (e.g., AWS ElastiCache or MemoryDB) so operators aren’t
misled—edit the description string associated with services.redis.local_setup to
replace “aws rds or similar services” with something like “AWS ElastiCache or
MemoryDB (or other managed Redis services)”.
---
Duplicate comments:
In `@charts/plane-enterprise/templates/service-account.yaml`:
- Around line 1-17: The template uses Sprig's merge which preserves existing
keys, so later overlays like .Values.irsa.roleArn and
.Values.podIdentity.roleArn can be blocked; change the overlay merges to use
mergeOverwrite so later values win. Concretely, keep the initial $annotations :=
dict and the base merge of .Values.serviceAccount.annotations, but replace the
merge calls that apply .Values.irsa.annotations, the
dict("eks.amazonaws.com/role-arn", .Values.irsa.roleArn), the
dict("eks.amazonaws.com/role-arn", .Values.podIdentity.roleArn), and
.Values.podIdentity.annotations with mergeOverwrite so the IRSA and Pod Identity
role ARNs and podIdentity annotations take precedence as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b47e278-6177-4723-b73a-1f7394c70f7d
📒 Files selected for processing (5)
charts/plane-enterprise/README.mdcharts/plane-enterprise/templates/config-secrets/live-env.yamlcharts/plane-enterprise/templates/config-secrets/silo.yamlcharts/plane-enterprise/templates/service-account.yamlcharts/plane-enterprise/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/plane-enterprise/templates/config-secrets/silo.yaml
- charts/plane-enterprise/values.yaml
| | Setting | Default | Required | Description | | ||
| | -------------------------------- | :-------------------------: | :------: | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | services.redis.local_setup | true | | Plane uses `valkey` to cache the session authentication and other static data. This database can be hosted within kubernetes as part of helm chart deployment or can be used as hosted service remotely (e.g. aws rds or similar services). Set this to `true` when you choose to setup stateful deployment of `redis`. Mark it as `false` when using a remotely hosted database | | ||
| | services.redis.local_setup | true | | Plane uses `valkey` to cache the session authentication and other static data. This database can be hosted within kubernetes as part of helm chart deployment or can be used as hosted service remotely (e.g. aws rds or similar services). Set this to `true` when you choose to setup stateful deployment of `redis`. Mark it as `false` when using a remotely hosted database. Important: if you configure Redis credentials via AWS Secrets Manager ARNs (for example `env.apiSecrets.elasticache_secret_arn`) and provide the Redis key mapping (`env.redis_auth_token_key`), keep this set to `false` so the chart does not compute `REDIS_URL` from in-cluster services. | |
Contributor
There was a problem hiding this comment.
Redis hosted-service example uses the wrong AWS service name
Line 160 references “aws rds” for Redis; this should be Redis-specific (e.g., AWS ElastiCache/MemoryDB) to avoid operator confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plane-enterprise/README.md` at line 160, The README text for the
services.redis.local_setup description incorrectly mentions “aws rds”; update
the wording to reference Redis-specific hosted services (e.g., AWS ElastiCache
or MemoryDB) so operators aren’t misled—edit the description string associated
with services.redis.local_setup to replace “aws rds or similar services” with
something like “AWS ElastiCache or MemoryDB (or other managed Redis services)”.
akshat5302
changed the title
v2.3.0v2.3.0
…cret ARN resolution. This update introduces fallback mechanisms for compatibility with older values files, ensuring smoother transitions and improved configuration management.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml`:
- Around line 119-122: The condition gating the RABBITMQ_* entries is
inconsistent with the ConfigMap: replace the direct check of
.Values.env.apiSecrets.amazonmq_secret_arn with the PI-resolved variable so both
use the same source of truth; specifically, update the if expression that
currently tests .Values.env.apiSecrets.amazonmq_secret_arn to test
$resolvedAmazonmqSecretArn (and keep the checks for
.Values.env.pi_envs.rabbitmq_password_key and
.Values.env.pi_envs.rabbitmq_user_key) so the Secret rendering for
AMAZONMQ_SECRET_ARN and the ConfigMap use the same resolved value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ed5ef91-250e-4ee3-8f36-52d72d7ddf3c
📒 Files selected for processing (1)
charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml
Comment on lines
+119
to
+122
| {{- if and (not (empty .Values.env.apiSecrets.amazonmq_secret_arn)) (not (empty .Values.env.pi_envs.rabbitmq_password_key)) (not (empty .Values.env.pi_envs.rabbitmq_user_key)) }} | ||
| RABBITMQ_PASSWORD_KEY: {{ .Values.env.pi_envs.rabbitmq_password_key | quote }} | ||
| RABBITMQ_USER_KEY: {{ .Values.env.pi_envs.rabbitmq_user_key | quote }} | ||
| {{- end }} |
Contributor
There was a problem hiding this comment.
Inconsistent condition: ConfigMap uses direct value while Secret uses resolved variable.
Line 76 uses $resolvedAmazonmqSecretArn (which respects the PI-specific override), but line 119 checks only .Values.env.apiSecrets.amazonmq_secret_arn. If a user sets only the PI-specific services.pi.apiSecrets.amazonmq_secret_arn, the Secret entry AMAZONMQ_SECRET_ARN will render, but these ConfigMap key mappings won't—causing a partial and likely broken configuration.
Proposed fix to use the resolved variable consistently
- {{- if and (not (empty .Values.env.apiSecrets.amazonmq_secret_arn)) (not (empty .Values.env.pi_envs.rabbitmq_password_key)) (not (empty .Values.env.pi_envs.rabbitmq_user_key)) }}
+ {{- if and (not (empty $resolvedAmazonmqSecretArn)) (not (empty .Values.env.pi_envs.rabbitmq_password_key)) (not (empty .Values.env.pi_envs.rabbitmq_user_key)) }}
RABBITMQ_PASSWORD_KEY: {{ .Values.env.pi_envs.rabbitmq_password_key | quote }}
RABBITMQ_USER_KEY: {{ .Values.env.pi_envs.rabbitmq_user_key | quote }}
{{- 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.
Suggested change
| {{- if and (not (empty .Values.env.apiSecrets.amazonmq_secret_arn)) (not (empty .Values.env.pi_envs.rabbitmq_password_key)) (not (empty .Values.env.pi_envs.rabbitmq_user_key)) }} | |
| RABBITMQ_PASSWORD_KEY: {{ .Values.env.pi_envs.rabbitmq_password_key | quote }} | |
| RABBITMQ_USER_KEY: {{ .Values.env.pi_envs.rabbitmq_user_key | quote }} | |
| {{- end }} | |
| {{- if and (not (empty $resolvedAmazonmqSecretArn)) (not (empty .Values.env.pi_envs.rabbitmq_password_key)) (not (empty .Values.env.pi_envs.rabbitmq_user_key)) }} | |
| RABBITMQ_PASSWORD_KEY: {{ .Values.env.pi_envs.rabbitmq_password_key | quote }} | |
| RABBITMQ_USER_KEY: {{ .Values.env.pi_envs.rabbitmq_user_key | quote }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/plane-enterprise/templates/config-secrets/pi-api-env.yaml` around
lines 119 - 122, The condition gating the RABBITMQ_* entries is inconsistent
with the ConfigMap: replace the direct check of
.Values.env.apiSecrets.amazonmq_secret_arn with the PI-resolved variable so both
use the same source of truth; specifically, update the if expression that
currently tests .Values.env.apiSecrets.amazonmq_secret_arn to test
$resolvedAmazonmqSecretArn (and keep the checks for
.Values.env.pi_envs.rabbitmq_password_key and
.Values.env.pi_envs.rabbitmq_user_key) so the Secret rendering for
AMAZONMQ_SECRET_ARN and the ConfigMap use the same resolved value.
pratapalakshmi
changed the title
v2.3.0v2.3.0
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Documentation
New Features