-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat: [ISSUE-1401]: Added helm get values option in Helm collector #1402
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.
Thanks a lot for your contribution!
The overall change looks good to me. I have added some comments, mostly around cleanup and some improvements I noticed that need to be done
The other missing bit that would be needed in this PR is
- Documenting the helm collector in this file
- Updating this e2e test. It will involve adding some values the helm test fixture and passing in some values which you will assert their existence here
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.
Error handling changes look good, thanks for that. I have added one code suggestion to the PR
Signed-off-by: Akash Shrivastava <[email protected]>
Signed-off-by: Akash Shrivastava <[email protected]>
Signed-off-by: Akash Shrivastava <[email protected]>
Docs PR: replicatedhq/troubleshoot.sh#534 |
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.
The PR changes work. I added one last comment needing to be changes
Signed-off-by: Akash Shrivastava <[email protected]>
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.
LGTM
Description, Motivation and Context
This PR adds the helm values collection enhancement to Helm collector.
For more information, refer to this issue: #1401
Checklist
Does this PR introduce a breaking change?