-
Notifications
You must be signed in to change notification settings - Fork 166
Helm install via yaml and node management #184
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
base: main
Are you sure you want to change the base?
Helm install via yaml and node management #184
Conversation
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.
Hi, I think that this is a good PR - requesting changes for 2 reasons:
- It needs tests for the new tools
- There's some discussion we should have around helm_template_install specifically because it's kinda a duplicate tool to fix errors in regular helm_install?
CC @rr-paras-patel - any comments around the cleanup & node_management tools?
src/tools/helm-template-apply.ts
Outdated
|
||
export const helmTemplateApplySchema = { | ||
name: "helm_template_apply", | ||
description: "Install a Helm chart using template generation and kubectl apply (bypasses authentication issues)", |
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.
I'm wondering if this description should be a bit more descriptive for an LLM - "Install a Helm chart using template generation and kubectl apply. Attempt to use this if helm_install has authentication issues or errors".
The other thing is if it would be better to replace the regular helm_install instead of having a new tool that essentially does the same thing?
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.
+1
- `kubectl_generic`: General kubectl command access (may include destructive operations) | ||
|
||
### Helm Template Apply Tool |
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.
I feel like this should be in some docs/helm_install.md and docs/cleanup_pods.md file (readme is already pretty long & describing every tool in the readme itself might be excessive)
|
||
#### Usage Examples | ||
|
||
**1. List all nodes:** |
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.
existing kubectl_get
cover this operation.
src/index.ts
Outdated
); | ||
} | ||
|
||
case "cleanup_pods": { |
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.
existing kubectl_delete
method should cover this. and LLM should be able to pass required params.
src/tools/cleanup-pods.ts
Outdated
} | ||
}; | ||
|
||
const getProblematicPods = (namespace: string, allNamespaces: boolean = false): { [key: string]: string[] } => { |
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.
Technically LLM should be able to filter such problematic pods instead of dedicated tool.
src/tools/helm-template-apply.ts
Outdated
|
||
export const helmTemplateApplySchema = { | ||
name: "helm_template_apply", | ||
description: "Install a Helm chart using template generation and kubectl apply (bypasses authentication issues)", |
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.
+1
Hi @Flux159 and @rr-paras-patel , FYI for : "here's some discussion we should have around helm_template_install specifically because it's kinda a duplicate tool to fix errors in regular helm_install?"
|
Couldn't it just be an optional argument for helm_install then? Just have two different codepaths in that tool based on the argument |
Hi @Flux159 , yes I will do the changes accordingly. |
…m-operations, improve cleanup-pods parsing, add comprehensive tests and documentation
…m-operations, improve cleanup-pods parsing, add comprehensive tests and documentation
e90a22d
to
c62a50b
Compare
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.
please address comments
Hi @rr-paras-patel , |
I have now ,
|
This reverts commit 1c47731.
…conflicts in helm-operations.ts
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.
Pull Request Overview
This PR adds Helm chart installation via YAML manifests and comprehensive node management capabilities to enhance Kubernetes resource management and maintenance operations.
- Introduces template-based Helm chart installation that bypasses authentication issues by using
helm template + kubectl apply
instead of directhelm install
- Adds comprehensive node management tool for cordoning, draining, and uncordoning operations with safety features
- Updates existing Helm operations with improved error handling and flexible installation modes
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
tests/node-management.test.ts |
Complete test suite for node management operations with schema validation and safety feature testing |
tests/helm.test.ts |
Enhanced integration tests with Helm availability checking and improved error handling with debug output |
tests/helm-operations.test.ts |
Unit tests for Helm operations schemas with parameter validation and template mode testing |
src/tools/node-management.ts |
New comprehensive node management tool with cordon, drain, uncordon operations and safety controls |
src/tools/helm-operations.ts |
Enhanced Helm operations with template mode support and improved error handling |
src/models/helm-models.ts |
Updated Helm models to support new optional parameters and error states |
src/index.ts |
Integration of new node management tool into the main server |
docs/node-management.md |
Complete documentation for node management operations and safety features |
docs/helm-install.md |
Documentation for enhanced Helm installation capabilities including template mode |
README.md |
Updated documentation with new features and comprehensive usage examples |
.github/workflows/ci.yml |
Added Helm installation to CI pipeline for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Hi @rr-paras-patel , the above suggestions made by copilot were very minor changes so I have commited them directly . |
Hi @rr-paras-patel @Flux159 , Please review. |
Adds support for installing Helm charts using YAML manifests, improving flexibility for Kubernetes resource management.
Updates and refines node management capabilities, including improved tools for node operations and better integration with the unified kubectl command set.
-fixed top command usage