Skip to content

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Nov 14, 2025

this PR is meant to be reference only and should not be merged.

Description

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

ports:
- port: 3306
targetPort: 3306
---
Copy link
Member Author

Choose a reason for hiding this comment

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

fine to allow --- in helm yamls?

app.kubernetes.io/component: "database"
ports:
- port: 3306
nodePort: 30306
Copy link
Member Author

Choose a reason for hiding this comment

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

shall we expose the port using NodePort, or ask users to run kubectl port-forward to expose ports explicitly for databases?

ports:
- port: 4000
targetPort: 4000
nodePort: 30400
Copy link
Member Author

Choose a reason for hiding this comment

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

shall we set up nginx ingress instead of this?

@@ -0,0 +1,17 @@
apiVersion: "v2"
name: "clp-package"
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. is this name fine?
  2. is the tools/deployment/package-helm name fine?

apiVersion: "v1"
kind: "ConfigMap"
metadata:
name: {{ include "clp-package.fullname" . }}-config
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe -generated-config?


storage:
# Base directory on host when using local PVs.
localPathBase: "/tmp/clp"
Copy link
Member Author

Choose a reason for hiding this comment

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

maybe just a clpHome key at the root? or we enforce users to specify absolute paths in the clpConfig

Copy link
Member Author

Choose a reason for hiding this comment

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

i believe it's better to enforce absolute paths in clpConfig, so that we don't have to do path manipulations with go templates. though that means users can't just directly copy over an already working clp-config.yml and paste into clpConfig - they will have to adjust all paths to use absolute paths

@junhaoliao junhaoliao changed the title feat(deployment): Add k8s Helm chart for CLP package deployment. WIP - feat(deployment): Add k8s Helm chart for CLP package deployment. Nov 14, 2025
port: 6379
query_backend_database: {{ .Values.clpConfig.redis.query_backend_database }}
reducer:
base_port: {{ .Values.clpConfig.reducer.base_port }}
Copy link
Member Author

Choose a reason for hiding this comment

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

technically we can hardcode this since we dont need to expose the ports outside of the cluster?

junhaoliao and others added 22 commits November 26, 2025 19:48
# Conflicts:
#	tools/deployment/package-helm/templates/configmap.yaml
#	tools/deployment/package-helm/templates/database-statefulset.yaml
#	tools/deployment/package-helm/test.sh
#	tools/deployment/package-helm/values.yaml
…goDB) as `StatefulSet` into the CLP Package Helm chart.
# Conflicts:
#	tools/deployment/package-helm/templates/_helpers.tpl
#	tools/deployment/package-helm/templates/configmap.yaml
#	tools/deployment/package-helm/test.sh
#	tools/deployment/package-helm/values.yaml
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