-
Notifications
You must be signed in to change notification settings - Fork 608
chore(k8s): restructure manifests, improve deployment stability, add GCP Simple Run #2256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ryszard-szwajlik-j-labs
wants to merge
4
commits into
SciPhi-AI:main
Choose a base branch
from
j-labs:chore/k8s-improvements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
chore(k8s): restructure manifests, improve deployment stability, add GCP Simple Run #2256
ryszard-szwajlik-j-labs
wants to merge
4
commits into
SciPhi-AI:main
from
j-labs:chore/k8s-improvements
Conversation
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
…with not-existing resources, fixed database urls and resources, added simple-run version for running in GCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 6612a2f in 2 minutes and 23 seconds. Click for details.
- Reviewed
1078lines of code in49files - Skipped
0files when reviewing. - Skipped posting
13draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. deployment/k8s/kustomizations/base/helm-values_hatchet.yaml:53
-
Draft comment:
Updated image tags to v0.72.8 are applied consistently. Please verify that the extraVolumes configuration for mounting the 'hatchet-keyset' secret meets your security requirements. -
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify security requirements, which is against the rules. It also mentions an update to image tags, which is a dependency change and should not be commented on.
2. deployment/k8s/kustomizations/base/helm-values_postgresql.yaml:23
-
Draft comment:
The POSTGRESQL_MAX_CONNECTIONS is set to '500'. Confirm that this capacity is appropriate for your workload and matches your pool sizing expectations. -
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding the configuration of POSTGRESQL_MAX_CONNECTIONS. This falls under the rule of not asking the author to confirm their intention or to ensure the behavior is intended. Therefore, this comment should be removed.
3. deployment/k8s/kustomizations/base/include/cm-init-scripts-hatchet.yaml:165
-
Draft comment:
The inject-secret.sh script uses '--insecure' with curl. Ensure that skipping certificate validation is acceptable for your cluster environment or consider enabling proper certificate verification. -
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. deployment/k8s/kustomizations/base/include/r2r-initc.yaml:70
-
Draft comment:
The fallback for HATCHET_CLIENT_TOKEN reads from /hatchet_api_key/api_key.txt, but the corresponding volume mount is commented out. Either un-comment the volume mount or ensure the token is provided via the secret. -
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. deployment/k8s/kustomizations/base/patches/hatchet-rabbitmq-sts.yaml:22
-
Draft comment:
RabbitMQ health probes use environment variables (${RABBITMQ_USERNAME} and ${RABBITMQ_PASSWORD}). Confirm these variables are properly set in the pod environment to ensure the probes function as intended. -
Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. deployment/k8s/kustomizations/overlays/gcp/base/helm-values_postgresql.yaml:10
-
Draft comment:
Storage class has been updated to 'standard' for GCP. Ensure that the 'standard' storage class is available in your GCP cluster and meets your performance and durability requirements. -
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that the 'standard' storage class is available and meets requirements, which is a form of asking for confirmation or verification. This violates the rule against asking the author to ensure or verify things.
7. deployment/k8s/kustomizations/overlays/gcp/dev/patches/cm-r2r.yaml:9
-
Draft comment:
Review the API endpoint and redirect URI settings in the r2r-configmap to ensure they match your deployment environment. -
Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
8. deployment/k8s/kustomizations/overlays/gcp/prod/README.md:1
-
Draft comment:
The production overlay README is brief. Consider providing additional instructions for integrating with a secrets manager (e.g. Bitwarden Secrets Manager) and other production-specific configurations. -
Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
9. deployment/k8s/kustomizations/overlays/gcp/prod/kustomization.yaml:1
-
Draft comment:
The production kustomization.yaml currently only references the base. Verify if additional patches or production-specific configurations are needed for a complete production deployment. -
Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
10. deployment/k8s/kustomizations/base/helm-values_postgresql.yaml:8
-
Draft comment:
Typographical note: Consider adding a space after the '#' in the comment. For example, change '#creates hatchet database' to '# creates hatchet database' to improve readability. -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a very minor stylistic suggestion about comment formatting. It doesn't affect functionality at all. The comment is readable even without the space. This kind of nitpicky formatting suggestion doesn't meet our bar for "clearly a code change required" and falls under "obvious or unimportant" category. Perhaps consistent comment formatting could be important for maintaining code style standards across the project? While consistent formatting is good, this is too minor to warrant a PR comment. If comment formatting is important, it should be handled by automated tooling or style guides, not manual review comments. Delete this comment as it's too minor and doesn't meet our criteria for keeping comments - it's not about a significant issue requiring code changes.
11. deployment/k8s/kustomizations/base/helm-values_postgresql.yaml:4
-
Draft comment:
Potential typo: The value for 'adminPasswordKey' is 'HATCHET_DATABASE_POSTGRES_POSTGRES_PASSWORD' (with 'POSTGRES' repeated). If this isn’t intentional, please verify if it should be 'HATCHET_DATABASE_POSTGRES_PASSWORD' to match the pattern in 'userPasswordKey'. -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Environment variable names are often carefully chosen and may have specific meanings. The double 'POSTGRES' could be intentional to distinguish between the postgres admin user password vs the regular user password. Without access to the actual secret configuration or documentation, we can't be certain this is a mistake. The comment is speculative and asks for verification rather than pointing out a clear issue. The repeated word could genuinely be a copy-paste error that should be fixed. Environment variable naming consistency is important for maintainability. While consistency is important, we don't have enough context to know if this naming is intentional. The comment violates the rule about not asking for verification or making speculative comments. Delete the comment as it's speculative and asks for verification rather than pointing out a definitive issue. We don't have strong evidence that this is actually incorrect.
12. deployment/k8s/kustomizations/overlays/gcp/base/kustomization.yaml:20
-
Draft comment:
The file is missing a trailing newline at the end. While this is a minor issue, it's best practice to add a newline at the end of the file for consistency. -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While missing trailing newlines can cause issues with some tools and it's generally good practice to have them, this is a very minor issue. Most modern editors and tools handle this automatically. The comment doesn't indicate any functional problems, and it's more of a style preference than a critical issue. The missing newline could potentially cause issues with some Git operations or text processing tools. It's a valid observation about code quality. However, this is exactly the kind of minor, obvious issue that our rules say we should not comment on. It's not clearly requiring a code change and is more informative than actionable. The comment should be deleted as it violates our rules about not making purely informative comments or commenting on obvious/unimportant issues.
13. deployment/k8s/kustomizations/overlays/gcp/dev/include/sec-r2r.yaml:14
-
Draft comment:
Typo check: The key 'LITELLM_PROXY_API_KEY' on line 14 looks unusual. Please verify if 'LITELLM' is the intended spelling or if it should be 'LITTLELM' or similar. -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% LITELLM is a real and legitimate service name - it's an open source tool for LLM API management. The comment is questioning something that isn't actually wrong. This falls under the category of comments that shouldn't make speculative suggestions without strong evidence of an issue. Could there be a different LLM proxy service that the author meant to use instead? Maybe the comment is valid if this is meant to be a different service? No - LITELLM is a specific, well-known tool in the LLM space, and given the context of other LLM-related API keys, this is clearly intentional. The comment should be deleted as it's questioning the spelling of a legitimate service name and doesn't point to any actual issue.
Workflow ID: wflow_N9IBz1bFo9sGPQDk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
📌 Description of Changes
This PR, prepared as part of the work done by the AI R&D team at J‑Labs, introduces a set of improvements and fixes to the Kubernetes configuration, aiming to improve clarity, maintainability, and deployment reliability:
kustomizationdirectory — clearer organization of manifests, improved separation of concerns, and cleaner file structure.hatchetversion and other related configuration values.simple-runGCP deployment variant — streamlined configuration for running the project in Google Cloud Platform with minimal setup, useful for lightweight deployments or testing.POSTGRESQL_MAX_CONNECTIONS— tuned the default database connection limits to better match expected load and avoid connection pool issues.READMEwith instructions on how to run the app using the updated Kubernetes setup.Deployment is now significantly more stable than it was prior to these changes. However, full stability will be achieved once this Helm chart update to Hatchet is released and integrated.
Until then, occasional manual redeployment of the
hatchet-apimay be required if it fails during startup.✅ What Has Been Tested
simple-runvariant on GCP — verify that this path deploys successfully and reliably.hatchetversion and other changes do not break functionality or introduce regressions.🔧 Next Steps / Suggestions
prod-overlay, which currently only contains what was there before this PR.🔍 Testing & Review
deployment/k8s/kustomizations/overlays/gcp/dev/README.mdto run the application.Important
Restructures Kubernetes manifests, adds GCP deployment variant, updates dependencies, and improves deployment stability.
kustomizationdirectory for clearer organization and separation of concerns.baseandoverlays/gcpdirectories for better structure.simple-runGCP deployment variant inoverlays/gcpfor streamlined setup.README.mdinoverlays/gcp/devwith setup instructions.hatchetversion tov0.72.8inhelm-values_hatchet.yamlandkustomization.yaml.cm-init-scripts-hatchet.yamlandcm-r2r.yaml.POSTGRESQL_MAX_CONNECTIONSto500inhelm-values_postgresql.yaml.overlays/gcp/dev/include.This description was created by
for 6612a2f. You can customize this summary. It will automatically update as commits are pushed.