Helm module refactors #6
Draft
Conversation
This file contains 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
- Restructure logging: * Add scopes with .for() method * Change .info logs to be informative feedback as to what is happening * Scope logs to .info and .debug categories more accurately * Delete many lines of unnecessary .info and .debug logs * Completely delete developer logs like random arguments or words - Change signatures of methods corresponding to helm operations (pull, install...) from accepting cli strings to accepting arguments (from which cli commands are built). Helm module has a place in this codebase as layer of abstraction over helm binary only when providing api that can be used declaratively. It doesn't have much sense when many of the methods are accepting complete cli commands, it defeats the purpose of this module (testsuite could just use Process.run in place of method calls) and makes calls to those methods completely unconsistent throughout the codebase. - Simplify methods without changing logic wherever possible - Delete module methods related to helm v2 support - Delete prints to stdout_ from module (moved to main cnti_testsuite) - there are still prints in utils.cr marked as TODO. - Run official crystal-lang autoformatter - Delete unused constants from top of the file Signed-off-by: Rafal Lal <[email protected]>
- Do other formatting operations as breaking long lines etc - Update according to changes in Helm module Signed-off-by: Rafal Lal <[email protected]>
13 tasks
Signed-off-by: Rafal Lal <[email protected]>
svteb
reviewed
Feb 7, 2025
| module ShellCmd | ||
| def self.run(cmd, log_prefix, force_output=false) | ||
| Log.info { "#{log_prefix} command: #{cmd}" } | ||
| module ShellCMD |
There was a problem hiding this comment.
what is the reason for renaming ShellCmd to ShellCMD? If this is to be done, the kubectl_client shard and the main testsuite repo also make use of a similar module which should be renamed in the future to retain conformity.
|
|
||
| Helm::ShellCmd.run("ls -alR #{helm_chart}", "before generate") | ||
| Helm::ShellCmd.run("ls -alR cnfs", "before generate") | ||
| # TODO: do we need to log those 'ls' commands? |
There was a problem hiding this comment.
Hard to tell, this function is no longer utilized in testsuite.
Comment on lines
+230
to
+236
| if (resp[:output] + resp[:error]) =~ /WARNING: Kubernetes configuration file is/ | ||
| return {true, "For this version of helm you must set your K8s config file permissions to chmod 700"} | ||
| end | ||
| rescue ex | ||
| stdout_failure("Please use newer version of helm") | ||
| true | ||
|
|
||
| return {false, nil} | ||
| rescue | ||
| return {true, "Please use newer version of helm"} |
There was a problem hiding this comment.
Could there be a better solution than returning tuples? Could they at least be named?
|
Additionally, we prefer that pull requests be limited to one commit, so consider squashing/rebasing them when you are done. |
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.
UNTESTED
Restructure logging:
Change signatures of methods corresponding to helm operations (pull,
install...) from accepting cli strings to accepting arguments (from
which cli commands are built).
Helm module has a place in this codebase as layer of abstraction over
helm binary only when providing api that can be used declaratively.
It doesn't have much sense when many of the methods are accepting
complete cli commands, it defeats the purpose of this module (testsuite
could just use Process.run in place of method calls) and makes calls
to those methods completely unconsistent throughout the codebase.
Simplify methods without changing logic wherever possible
Delete module methods related to helm v2 support
Delete prints to stdout_ from module (moved to main cnti_testsuite) -
there are still prints in utils.cr marked as TODO.
Run official crystal-lang autoformatter
Delete unused constants from top of the file