Reorganize structs in different files #19
Conversation
nicholastmosher
force-pushed
the
split-modules
branch
from
5bd2115 to
e362e11
Compare
| /// | ||
| /// This only succeeds if the helm command can be found. | ||
| pub fn new() -> Result<Self, HelmError> { | ||
| let output = Command::new("helm").arg("version").result()?; |
|
|
||
| /// Adds a new helm repo with the given chart name and chart location | ||
| #[instrument(skip(self))] | ||
| pub fn repo_add(&self, chart: &str, location: &str) -> Result<(), HelmError> { |
There was a problem hiding this comment.
Seems like repo related commands should be move to it's own builder type?
There was a problem hiding this comment.
Tracking feedback in #21
I think it would be good to tackle this in another PR since this one is purely refactoring
|
|
||
| /// Searches the repo for the named helm chart | ||
| #[instrument(skip(self))] | ||
| pub fn search_repo(&self, chart: &str, version: &str) -> Result<Vec<Chart>, HelmError> { |
| /// Get all the available versions | ||
| #[instrument(skip(self))] | ||
| pub fn versions(&self, chart: &str) -> Result<Vec<Chart>, HelmError> { | ||
| let mut command = Command::new("helm"); |
There was a problem hiding this comment.
Maybe this should be moved into Charts objects?
There was a problem hiding this comment.
Created this to track this feedback: #20
|
|
||
| /// Checks that a given version of a given chart exists in the repo. | ||
| #[instrument(skip(self))] | ||
| pub fn chart_version_exists(&self, name: &str, version: &str) -> Result<bool, HelmError> { |
There was a problem hiding this comment.
similar to repo, I think we should move all chart related commands to Chart object
|
|
||
| /// Installer Argument | ||
| #[derive(Debug)] | ||
| pub struct InstallArg { |
There was a problem hiding this comment.
Maybe use builder to reduce boiler plate?
|
|
||
| /// Uninstaller Argument | ||
| #[derive(Debug)] | ||
| pub struct UninstallArg { |
There was a problem hiding this comment.
Suggest have different types of builder. For example, helm.install_builder(), helm.uninstall_builder(). etc
|
Hi @sehz, this PR is strictly a refactoring, there is no change to the structs or functions that are currently in master. This was put together by @andreclaudino to help clean things up a bit. They have also made many more commits on their own fork of I am trying to work through their commits a little bit at a time, starting with this one that is simply breaking out modules without any actual code changes. I believe further commits have actual improvements such as additional features or improvements to the API (that may solve some of the problems you have mentioned here). I think it'd be a good idea to merge this PR as a solid incremental improvement and we can turn some of your comments into issues that we'd eventually like to address in this project. |
This is the first part of merging @andreclaudino's changes from #14. This first commit is a lot of moving code around, which was making it hard to review other meaningful changes. I've looked over this and everything looks good, hopefully by merging it we'll be able to reduce the diff noise in #14.