Skip to content

fix: questions and values file for CE and EE#65

Merged
mguptahub merged 12 commits intodevelopfrom
fix-questions-file
Dec 2, 2024
Merged

fix: questions and values file for CE and EE#65
mguptahub merged 12 commits intodevelopfrom
fix-questions-file

Conversation

@akshat5302
Copy link
Copy Markdown
Member

@akshat5302 akshat5302 commented Dec 2, 2024

Changed pullpolicy for application to Always and third-party applications to IfNotPresent in both CE and EE

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced installation instructions and configuration settings for deploying the Plane application using Helm.
    • Added new pullPolicy configuration options for various services, allowing for more flexible image management.
  • Bug Fixes

    • Removed deprecated postgres.cliConnectPort setting to simplify PostgreSQL service configuration.
  • Documentation

    • Improved clarity and usability of the README files, emphasizing environment variables and setup instructions.
    • Organized configuration settings for better user guidance on service deployments.
  • Chores

    • Updated default image pull policies across multiple services to ensure the latest images are used during deployments.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Dec 2, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The changes in this pull request involve significant updates to the documentation and configuration files for the Plane application deployment using Helm on Kubernetes. Key modifications include enhanced installation instructions in the README files, the introduction of new pull policy variables for various services, and the removal of the postgres.cliConnectPort setting. Additionally, the image pull policy for multiple deployments has been standardized to allow for dynamic configuration via Helm values, improving the overall clarity and usability of the deployment process.

Changes

File Path Change Summary
charts/plane-ce/README.md Updated installation instructions and configuration settings for deploying Plane with Helm, including detailed sections for components like Postgres, Redis, RabbitMQ, and SSL setup. Removed postgres.cliConnectPort.
charts/plane-ce/questions.yml Added new pullPolicy variables for Redis, Postgres, RabbitMQ, and Minio; updated defaults from "IfNotPresent" to "Always" for several services; removed postgres.cliConnectPort.
charts/plane-ce/templates/workloads/*.yaml Updated imagePullPolicy from "IfNotPresent" to "Always" for multiple deployments (admin, api, beat-worker, live, migrator, space, web, worker) to ensure the latest images are always pulled. Removed NodePort service for PostgreSQL CLI connections.
charts/plane-ce/values.yaml Removed cliConnectPort from the Postgres section; no other changes to the structure.
charts/plane-enterprise/README.md Enhanced installation instructions, emphasizing environment variable settings; reorganized setup sections for clarity.
charts/plane-enterprise/questions.yml Added pullPolicy variables for various services; removed cliConnectPort from Postgres.
charts/plane-enterprise/templates/workloads/*.yaml Updated imagePullPolicy to use dynamic values from .Values for several services (admin, api, beat-worker, live, migrator, minio, monitor, postgres, rabbitmq, redis, space, web, worker) to enhance configurability.
charts/plane-enterprise/values.yaml Introduced pullPolicy attributes for multiple services; removed cliConnectPort from Postgres.

Possibly related PRs

Suggested reviewers

  • sriramveeraghanta

🐰 "In the fields where the bunnies play,
New settings bloom, bright as day.
Helm charts updated, clear and bright,
Deploy with ease, all feels just right!
With pull policies set to 'Always' now,
We hop with joy, take a bow!" 🐇✨


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 852741a and ad106e1.

📒 Files selected for processing (5)
  • charts/plane-ce/README.md (10 hunks)
  • charts/plane-ce/questions.yml (14 hunks)
  • charts/plane-ce/values.yaml (1 hunks)
  • charts/plane-enterprise/README.md (14 hunks)
  • charts/plane-enterprise/questions.yml (13 hunks)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@akshat5302 akshat5302 marked this pull request as draft December 2, 2024 09:09
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (10)
charts/plane-enterprise/templates/workloads/minio.stateful.yaml (1)

Line range hint 108-108: Standardize image pull policy for MinIO client container

The MinIO client container still uses a hardcoded imagePullPolicy: Always, which is inconsistent with the PR's objective to make pull policies configurable and set third-party applications to IfNotPresent.

Apply this diff to maintain consistency:

-          imagePullPolicy: Always
+          imagePullPolicy: {{ .Values.services.minio.pullPolicy | default "IfNotPresent" }}

Additionally, consider adding a pull policy for the initContainer using busybox image to maintain consistency across all containers in this deployment.

charts/plane-enterprise/templates/workloads/monitor.stateful.yaml (1)

Line range hint 44-51: Consider parameterizing resource requests and reviewing resource ratios

While the resource limits are properly parameterized, there are two potential improvements:

  1. The resource requests are hardcoded. Consider parameterizing them similar to limits for consistency and flexibility.
  2. The ratio between requests and limits is quite large (20x for memory, 10x for CPU), which might lead to resource allocation inefficiencies.

Consider applying this change:

        resources:
          requests:
-           memory: "50Mi"
-           cpu: "50m"
+           memory: {{ .Values.services.monitor.memoryRequest | default "50Mi" | quote }}
+           cpu: {{ .Values.services.monitor.cpuRequest | default "50m" | quote }}
          limits:
            memory: {{ .Values.services.monitor.memoryLimit  | default "1000Mi" | quote }}
            cpu: {{ .Values.services.monitor.cpuLimit | default "500m" | quote}}

Consider adjusting the default resource ratios to be closer to recommended practices:

  • Memory: A ratio of 2-3x between limits and requests is typical
  • CPU: A ratio of 1.5-2x between limits and requests is common
charts/plane-enterprise/templates/workloads/migrator.job.yaml (1)

21-21: Consider standardizing values path structure between CE and EE.

While the change to use Always as default is correct, there's an inconsistency in the values path structure:

  • CE uses: .Values.api.pullPolicy
  • EE uses: .Values.services.api.pullPolicy

This might cause confusion during maintenance.

Consider standardizing the values path structure across CE and EE versions for better maintainability.

charts/plane-ce/templates/workloads/web.deployment.yaml (1)

43-43: LGTM! Consider updating documentation.

The change to use Always as default pull policy is correct for the frontend component as it's part of the main application. The templating allows for override via values while maintaining the desired default.

Consider updating the deployment documentation to explain the rationale behind different pull policies for main vs third-party components.

charts/plane-ce/templates/workloads/space.deployment.yaml (1)

Line range hint 50-51: Fix incorrect resource limit references.

The resource limits are referencing web.* values instead of space.* values, which appears to be incorrect:

-            memory: {{ .Values.web.memoryLimit  | default "1000Mi" | quote }}
-            cpu: {{ .Values.web.cpuLimit | default "500m" | quote}}
+            memory: {{ .Values.space.memoryLimit  | default "1000Mi" | quote }}
+            cpu: {{ .Values.space.cpuLimit | default "500m" | quote}}
charts/plane-ce/templates/workloads/admin.deployment.yaml (1)

Line range hint 21-21: Consider differentiating pull policies based on component type.

The current implementation sets imagePullPolicy: Always as the default for all components. However, according to the PR objectives, third-party applications should use IfNotPresent. Consider:

  1. Documenting which components are considered "main" vs "third-party"
  2. Setting appropriate defaults based on this classification
  3. Adding validation in the Helm chart to ensure correct policies are used

Also applies to: 43-43, 43-43

charts/plane-enterprise/templates/workloads/web.deployment.yaml (1)

43-43: LGTM! Consider documenting the configuration

The imagePullPolicy configuration aligns with the PR objectives, defaulting to "Always" for the web component while remaining configurable.

Consider adding a comment in the values.yaml file documenting this new configuration option and its implications:

services:
  web:
    # pullPolicy: Determines the image pull policy for the web component
    # - "Always": Always pull the image (recommended for development)
    # - "IfNotPresent": Use cached image if available
    pullPolicy: Always
charts/plane-enterprise/templates/workloads/live.deployment.yaml (1)

43-43: Consider standardizing value paths between CE and EE

While the changes are functionally correct, there's an inconsistency in the value paths:

  • CE uses: .Values.live.pullPolicy
  • EE uses: .Values.services.live.pullPolicy

Consider standardizing these paths for better maintainability.

charts/plane-enterprise/values.yaml (1)

92-92: Remove trailing whitespace.

There is a trailing whitespace at the end of line 92.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 92-92: trailing spaces

(trailing-spaces)

charts/plane-enterprise/questions.yml (1)

31-31: Remove trailing spaces

There are trailing spaces at the end of these lines that should be removed for consistency.

-    default: "registry.plane.tools/plane/web-enterprise"    
+    default: "registry.plane.tools/plane/web-enterprise"
-  
+
-  
+

Also applies to: 42-42, 361-361

🧰 Tools
🪛 yamllint (1.35.1)

[error] 31-31: trailing spaces

(trailing-spaces)

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a023f98 and 852741a.

📒 Files selected for processing (28)
  • charts/plane-ce/README.md (0 hunks)
  • charts/plane-ce/questions.yml (9 hunks)
  • charts/plane-ce/templates/workloads/admin.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/api.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/beat-worker.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/live.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/migrator.job.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/postgres.stateful.yaml (0 hunks)
  • charts/plane-ce/templates/workloads/space.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/web.deployment.yaml (1 hunks)
  • charts/plane-ce/templates/workloads/worker.deployment.yaml (1 hunks)
  • charts/plane-ce/values.yaml (0 hunks)
  • charts/plane-enterprise/README.md (0 hunks)
  • charts/plane-enterprise/questions.yml (8 hunks)
  • charts/plane-enterprise/templates/workloads/admin.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/api.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/beat-worker.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/live.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/migrator.job.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/minio.stateful.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/monitor.stateful.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/postgres.stateful.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/rabbitmq.stateful.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/redis.stateful.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/space.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/web.deployment.yaml (1 hunks)
  • charts/plane-enterprise/templates/workloads/worker.deployment.yaml (1 hunks)
  • charts/plane-enterprise/values.yaml (5 hunks)
💤 Files with no reviewable changes (4)
  • charts/plane-ce/templates/workloads/postgres.stateful.yaml
  • charts/plane-ce/README.md
  • charts/plane-ce/values.yaml
  • charts/plane-enterprise/README.md
🧰 Additional context used
🪛 yamllint (1.35.1)
charts/plane-enterprise/templates/workloads/beat-worker.deployment.yaml

[warning] 21-21: too many spaces after colon

(colons)

charts/plane-enterprise/questions.yml

[error] 31-31: trailing spaces

(trailing-spaces)


[error] 42-42: trailing spaces

(trailing-spaces)


[error] 361-361: trailing spaces

(trailing-spaces)

charts/plane-enterprise/values.yaml

[error] 92-92: trailing spaces

(trailing-spaces)

🔇 Additional comments (21)
charts/plane-enterprise/templates/workloads/minio.stateful.yaml (1)

43-43: LGTM! Proper implementation of configurable pull policy

The change correctly implements a configurable image pull policy with a default fallback to "IfNotPresent", which aligns with the PR objectives for third-party applications.

charts/plane-enterprise/templates/workloads/monitor.stateful.yaml (1)

40-40: Verify if monitor is a main or third-party application

The PR objectives specify different pull policies for main applications (Always) vs third-party applications (IfNotPresent). While the default value of Always is set here, we should verify if the monitor component is considered a main or third-party application to ensure this aligns with the PR objectives.

✅ Verification successful

Based on the results, I can now generate the final response:

Pull policy configuration is correct for monitor component

The monitor component is part of Plane's main application suite, as evidenced by:

  • Image path registry.plane.tools/plane/monitor-enterprise indicates it's a Plane-owned component
  • Default pull policy of Always in values.yaml aligns with PR objectives for main applications
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if monitor is referenced in other main application configurations
# or if it's part of third-party services

# Look for monitor service references
rg -l "monitor" charts/ | grep -v "monitor.stateful.yaml"

# Check the values file for service categorization
fd -t f "values.yaml" charts/ -x cat {} | grep -A 5 "monitor:"

Length of output: 506

charts/plane-ce/templates/workloads/migrator.job.yaml (1)

21-21: LGTM! Verify values file configuration.

The change to use {{ .Values.api.pullPolicy | default "Always" }} aligns with the PR objectives for main application components. Since this migrator job uses the same image as the API service, maintaining consistent pull policy is crucial for version compatibility.

✅ Verification successful

Verified: imagePullPolicy configuration is correctly aligned

The values file confirms that api.pullPolicy is set to "Always" by default in charts/plane-ce/values.yaml, which matches perfectly with the migrator job's template fallback. This ensures consistent image pulling behavior between the API service and its migrator job.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the values file contains the corresponding pullPolicy configuration
rg -l "api:" | xargs rg "pullPolicy:"

Length of output: 1212


Script:

#!/bin/bash
# Let's check the specific api.pullPolicy value in the CE values file
cat charts/plane-ce/values.yaml | grep -A 5 "^api:"

Length of output: 165

charts/plane-ce/templates/workloads/worker.deployment.yaml (1)

21-21: LGTM! Verify consistency across values files.

The imagePullPolicy configuration looks correct, defaulting to "Always" as intended for main application components.

Let's verify the corresponding values are properly defined:

✅ Verification successful

After analyzing the values files and templates, I can generate the final response:

The worker.pullPolicy configuration is properly aligned with backend components

The worker deployment inherits its image and pullPolicy from the backend configuration:

  • The worker uses the same backend image as defined in api.image
  • The pullPolicy default of "Always" matches other backend components
  • The values.yaml correctly defines worker configuration under the backend section
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if worker.pullPolicy is defined in values files
echo "Checking values files for worker.pullPolicy definition..."
fd -e yaml -e yml values.yaml questions.yaml -x cat {} \; | grep -A 2 "worker:"

Length of output: 309


Script:

#!/bin/bash
# Let's try a more targeted approach to find values files
echo "Looking for values files in the charts directory..."
fd -e yaml -e yml . charts/plane-ce -x sh -c 'echo "=== {} ==="; cat {}'

Length of output: 54083

charts/plane-ce/templates/workloads/space.deployment.yaml (1)

43-43: LGTM! Verify values configuration.

The imagePullPolicy configuration looks correct, defaulting to "Always" as intended for main application components.

Let's verify the corresponding values are properly defined:

✅ Verification successful

Verified: imagePullPolicy configuration is properly defined

The space.pullPolicy value is correctly defined in both CE and Enterprise Helm charts with "Always" as the default value, which matches the template configuration. The values are properly structured and aligned with the deployment manifest.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if space.pullPolicy is defined in values files
echo "Checking values files for space.pullPolicy definition..."
fd -e yaml -e yml values.yaml questions.yaml -x cat {} \; | grep -A 2 "space:"

Length of output: 306


Script:

#!/bin/bash
# Let's try a more comprehensive search for values files
echo "Looking for values.yaml files..."
fd values.yaml

echo -e "\nChecking content of values files for space configuration..."
fd values.yaml -x cat {} \; | grep -B2 -A5 "space:"

Length of output: 664

charts/plane-ce/templates/workloads/admin.deployment.yaml (1)

43-43: LGTM! Verify values configuration.

The imagePullPolicy configuration looks correct, defaulting to "Always" as intended for main application components.

Let's verify the corresponding values are properly defined:

✅ Verification successful

LGTM! Values configuration is properly defined

The imagePullPolicy configuration for the admin component is correctly defined in both CE and Enterprise charts:

  • In values.yaml, admin.pullPolicy is set to "Always" by default
  • In questions.yml, admin.pullPolicy is configurable via UI with "Always" as default
  • The template correctly references the value at .Values.admin.pullPolicy
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if admin.pullPolicy is defined in values files
echo "Checking values files for admin.pullPolicy definition..."
fd -e yaml -e yml values.yaml questions.yaml -x cat {} \; | grep -A 2 "admin:"

Length of output: 306


Script:

#!/bin/bash
# Let's try a more comprehensive search for values files
echo "Looking for values.yaml files in the charts directory..."
fd -e yaml -e yml . charts/ -x sh -c 'echo "=== {} ==="; cat {}'

Length of output: 68640

charts/plane-ce/templates/workloads/beat-worker.deployment.yaml (1)

21-21: LGTM! Verify values.yaml configuration

The imagePullPolicy configuration aligns with the PR objectives, defaulting to "Always" for this main application component while remaining configurable via Helm values.

✅ Verification successful

✓ Configuration verified - imagePullPolicy correctly set

The beatworker's imagePullPolicy is properly configured in values.yaml with "Always" as the value, which matches the default value set in the deployment template. This ensures consistent image pulling behavior for the beatworker component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the beatworker pullPolicy configuration in values.yaml
rg -l "beatworker:" | xargs rg "pullPolicy:"

Length of output: 1219


Script:

#!/bin/bash
# Let's check the specific beatworker configuration in values.yaml
rg -A 5 "beatworker:" charts/plane-ce/values.yaml

Length of output: 168

charts/plane-enterprise/templates/workloads/space.deployment.yaml (1)

43-43: LGTM! Verify space service configuration in values file.

The dynamic imagePullPolicy configuration with default "Always" aligns with the PR objectives for main application components.

✅ Verification successful

The space service configuration is properly defined in values.yaml

The values.yaml file already has the space service configuration with pullPolicy: Always set, which matches the default value in the deployment template. The configuration is complete and correct.

  • Location: charts/plane-enterprise/values.yaml
  • Current configuration:
    services:
      space:
        replicas: 1
        memoryLimit: 1000Mi
        cpuLimit: 500m
        image: registry.plane.tools/plane/space-enterprise
        pullPolicy: Always
        assign_cluster_ip: false
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the space service pull policy configuration exists in values file
rg "space:\s*\n\s*pullPolicy:" charts/plane-enterprise/values.yaml

Length of output: 260


Script:

#!/bin/bash
# Check space service configuration in values.yaml with multiline support
rg -U "space:\s*\n\s*pullPolicy:" charts/plane-enterprise/values.yaml

# Also check the entire space service configuration block
rg -U "services:\s*\n\s*space:" -A 10 charts/plane-enterprise/values.yaml

Length of output: 147


Script:

#!/bin/bash
# Let's check the values.yaml file content directly to understand the structure
cat charts/plane-enterprise/values.yaml

# Also check if there are any other files that might contain space service configuration
fd values.yaml

Length of output: 3613

charts/plane-enterprise/templates/workloads/admin.deployment.yaml (1)

43-43: LGTM! Verify admin service configuration in values file.

The dynamic imagePullPolicy configuration with default "Always" aligns with the PR objectives for main application components.

✅ Verification successful

Configuration verified - imagePullPolicy is properly configured

The admin service configuration in values.yaml has pullPolicy: Always set by default, which aligns perfectly with the template's fallback value. The configuration is properly structured within the services section and matches the deployment template's expectations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the admin service pull policy configuration exists in values file
rg "admin:\s*\n\s*pullPolicy:" charts/plane-enterprise/values.yaml

Length of output: 260


Script:

#!/bin/bash
# Check admin service configuration in values.yaml with multiline support
rg -U "admin:\s*\n\s*pullPolicy:" charts/plane-enterprise/values.yaml

# Also check the entire admin service configuration block
rg -U "services:\s*\n\s*admin:" -A 10 charts/plane-enterprise/values.yaml

Length of output: 147


Script:

#!/bin/bash
# Let's check the values.yaml file content directly to understand the structure
cat charts/plane-enterprise/values.yaml

# Also check if there are any other files that might contain admin service configuration
fd values.yaml

Length of output: 3613

charts/plane-ce/templates/workloads/live.deployment.yaml (2)

43-43: LGTM: Pull policy change aligns with PR objectives

The change to make imagePullPolicy configurable with a default of "Always" aligns with the PR objective for main application components.


Line range hint 48-53: Verify resource limits are sufficient for production workloads

The memory and CPU configurations have been made configurable with defaults:

  • Memory: request 50Mi, limit 1000Mi
  • CPU: request 50m, limit 500m

Please ensure these values are sufficient for production workloads.

✅ Verification successful

Let me generate the final response based on the findings.

Resource limits are consistent with documented defaults and configurable for production needs

The memory and CPU configurations in the live deployment match the documented defaults in the Helm chart's README.md. These values are configurable via Helm values, allowing operators to adjust them based on their specific production requirements:

  • Memory: request 50Mi, limit 1000Mi (configurable via live.memoryLimit)
  • CPU: request 50m, limit 500m (configurable via live.cpuLimit)

These defaults are consistent across all deployments in the chart (web, space, admin, live, api, worker, beatworker) suggesting they've been standardized based on general usage patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing resource usage documentation or recommendations
rg -g '*.md' -i 'resource|memory|cpu' charts/plane-ce/

Length of output: 3385

charts/plane-enterprise/templates/workloads/redis.stateful.yaml (2)

42-42: LGTM: Pull policy change aligns with PR objectives

The change to make imagePullPolicy default to "IfNotPresent" for Redis aligns with the PR objective for third-party components.


Line range hint 54-67: Verify volume configuration for Redis persistence

The StatefulSet includes volume configuration with:

  • ReadWriteOnce access mode
  • Configurable volume size (default: 1Gi)
  • Uses environment-specific storage class

Please ensure:

  1. The default volume size is sufficient for Redis data
  2. The storage class supports the required access mode
✅ Verification successful

Based on the search results, I can now generate the final response since I have found documentation about Redis volume configuration in the README.md file.

Redis volume configuration appears properly configured with documented defaults

The default volume size of 500Mi (not 1Gi as in the code) and storage class configuration are documented in the chart's README. The configuration looks appropriate because:

  • The default volume size of 500Mi should be sufficient for Redis data in most cases, as Redis is primarily used as a cache
  • The storage class is configurable via env.storageClass which allows administrators to specify a storage class that supports ReadWriteOnce access mode
  • The configuration follows the same pattern as other stateful services in the chart (Postgres, RabbitMQ, etc.)

However, note that the default volume size in the code (1Gi) differs from the documentation (500Mi). This provides even more headroom than documented.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Redis-specific configuration documentation
rg -g '*.md' -i 'redis.*storage|volume|persistence' charts/plane-enterprise/

Length of output: 2199

charts/plane-enterprise/templates/workloads/postgres.stateful.yaml (1)

39-39: LGTM: Pull policy change aligns with PR objectives

The change to use IfNotPresent as default for PostgreSQL (third-party service) is correct and matches the PR objectives.

Let's verify the values file configuration:

✅ Verification successful

Pull policy configuration is properly structured across chart files

The verification confirms that:

  • Both CE and Enterprise charts have proper configuration for PostgreSQL pull policy in questions.yml
  • The StatefulSet templates correctly use the pull policy with default "IfNotPresent"
  • Enterprise chart uses services.postgres.pullPolicy while CE uses postgres.pullPolicy, maintaining consistent namespacing within their respective charts
  • The default value of "IfNotPresent" is appropriate for a third-party service like PostgreSQL
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the postgres pull policy is properly configured in values files
rg -l "postgres.*pullPolicy" charts/

Length of output: 232


Script:

#!/bin/bash
# Check the actual configuration in values files and templates
echo "=== Checking questions.yml files ==="
rg "postgres.*pullPolicy" charts/plane-enterprise/questions.yml charts/plane-ce/questions.yml -A 2

echo -e "\n=== Checking StatefulSet templates ==="
rg "pullPolicy" charts/plane-enterprise/templates/workloads/postgres.stateful.yaml charts/plane-ce/templates/workloads/postgres.stateful.yaml -A 1

echo -e "\n=== Checking values.yaml files ==="
rg "postgres.*pullPolicy" charts/plane-enterprise/values.yaml charts/plane-ce/values.yaml

Length of output: 1492

charts/plane-ce/templates/workloads/api.deployment.yaml (1)

43-43: LGTM: Pull policy change aligns with PR objectives

The change to use Always as default for the API service (main application) is correct and matches the PR objectives.

Let's verify the values file configuration and check for consistency across other main application services:

✅ Verification successful

Pull policy configuration is consistent across all main application services

The verification confirms that:

  • All main application services (api, web, worker, space, live, admin, beat-worker) consistently use the same pull policy pattern: .Values.<service>.pullPolicy | default "Always"
  • Both CE and Enterprise charts follow the same configuration pattern
  • The values files contain the necessary pullPolicy configurations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check pull policy configuration for main application services
echo "Checking pull policy configuration in values files..."
rg -l "pullPolicy.*Always" charts/

echo "Checking consistency across main application services..."
rg -l "makeplane/plane-" charts/ | while read -r file; do
  echo "Checking $file..."
  rg "imagePullPolicy.*Always" "$file" || echo "Missing Always pull policy in $file"
done

Length of output: 5246

charts/plane-enterprise/templates/workloads/api.deployment.yaml (1)

43-43: LGTM! Appropriate image pull policy configuration.

The change correctly implements the configurable image pull policy with a default of "Always", which is suitable for the main application container to ensure the latest version is always pulled.

charts/plane-enterprise/values.yaml (1)

36-36: LGTM! Well-structured pull policy configuration.

The pull policy configuration follows a clear pattern:

  • "IfNotPresent" for infrastructure services (redis, postgres, rabbitmq, minio)
  • "Always" for application services (web, monitor, space, admin, live, api)

This strategy optimizes for both stability (infrastructure) and latest updates (application).

Also applies to: 44-44, 53-53, 64-64, 74-74, 81-81, 90-90, 98-98, 106-106, 114-114

charts/plane-ce/questions.yml (1)

256-263: LGTM! Well-structured pull policy configuration UI.

The pull policy configuration UI is consistently implemented across all services with appropriate default values:

  • Infrastructure services default to "IfNotPresent"
  • Clear labels and enum options

Also applies to: 302-309, 362-369, 430-437

charts/plane-enterprise/questions.yml (3)

75-82: LGTM: Pull policy configuration for main application services

The pull policy configuration for the main application services (web, space, admin, live, monitor, api) is correctly set to "Always" by default, which aligns with the PR objectives. The configuration is well-structured with appropriate enum options.

Also applies to: 102-109, 129-136, 156-163, 194-201, 221-228


306-313: LGTM: Pull policy configuration for third-party services

The pull policy configuration for third-party services (redis, postgres, rabbitmq, minio) is correctly set to "IfNotPresent" by default, which aligns with the PR objectives. The configuration includes proper show/hide conditions based on local_setup flags.

Also applies to: 347-355, 404-411, 458-465


347-361: Verify the removal of postgres.cliConnectPort

The AI summary mentions the removal of postgres.cliConnectPort. Let's verify if this removal has any implications for existing deployments.

✅ Verification successful

The removal of postgres.cliConnectPort is safe

Based on the git diff and configuration files analysis:

  1. The cliConnectPort was only used in the questions.yml file and has been safely removed
  2. The postgres service configuration only uses servicePort (5432) for database connectivity
  3. No other parts of the codebase reference or depend on the removed cliConnectPort setting
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of postgres.cliConnectPort in the codebase and deployment configurations

# Search for references to cliConnectPort
echo "Searching for references to postgres.cliConnectPort..."
rg -l "cliConnectPort"

# Check deployment templates for postgres port configurations
echo "Checking deployment templates..."
fd -e yaml -e tpl . charts/ --exec rg "port.*postgres|postgres.*port"

Length of output: 387


Script:

#!/bin/bash
# Let's check the git history for the removal of cliConnectPort
echo "Checking git history for the specific file..."
git diff HEAD~1 charts/plane-enterprise/questions.yml

# Let's also check for any references in values files
echo "Checking values files..."
fd values.yaml charts/ --exec cat {}

# Check templates for postgres service configurations
echo "Checking postgres service configurations..."
fd -e yaml -e tpl . charts/ --exec grep -l "postgres" {} | xargs cat

Length of output: 25879

🧰 Tools
🪛 yamllint (1.35.1)

[error] 361-361: trailing spaces

(trailing-spaces)

@mguptahub mguptahub marked this pull request as ready for review December 2, 2024 12:29
@mguptahub mguptahub merged commit dd7ffc2 into develop Dec 2, 2024
@mguptahub mguptahub deleted the fix-questions-file branch December 2, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants