Refactor the Helm Client (#26)

* Refactor and Testing Augmentations

Refactored the helm client commands to allow for better isolation
between classes and easier unit testing. Also refactored some of the
dependent classes to consume a helm client vs instantiate one. This will
hopefully increase the ability to test each component for subtle bugs.

See Changelog for more information.

* Added tests around provider to assure it accepts HelmCommand and outputs HelmCmdResponse

* Added test for not implemented

* Adjusted verbosity of logging to be more palatable

* Adjusted Client

Since we're not in static type land we need to validate we
instantiated with an iterator for default_helm_arguments.
So now we allow to be instantiated with `None` and convert
to `[]` and all other non-iterators raise an error.

* Fixed problem calling exception method

* Adjustments to docs and fixed a test confusion

* Removed the always pass circle ci release

.circleci/config.yml

@@ -58,7 +58,7 @@ jobs:
             echo "Using Repo:$GITHUB_REPOSITORY and Org:$GITHUB_ORGANIZATION"
             git fetch --tags
             curl -O https://raw.githubusercontent.com/reactiveops/release.sh/v0.0.2/release
-            /bin/bash release || true
+            /bin/bash release
       - run:
           name: package and upload
           command: |

.gitignore

@@ -4,3 +4,5 @@
 venvbuild
 dist
 venv
+.coverage
+htmlcov/

CHANGELOG.md

@@ -4,6 +4,25 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [0.10.0]
+
+Major-ish refactor to class structure and testing of independent classes in code. More work will be coming to make test coverage better.
+
+### Added
+- Default arguments for helm now do the right thing if using global flags for connecting to tiller (`tiller-namespace`, etc)
+
+### Fixed
+- Bubbled up actual error messages from Helm commands if they have a non-zero exit code
+
+### Breaking Changes
+- None expected but this is a refactor of the helm client interface
+
+### Notes
+Found several bugs that are noted in code and here. Fixes will be forthcoming.
+- Found that rollback functionality isn't operational
+- Found that dependency_update is non-operational
+- Found that kubectl context update isn't functional
+
 ## [0.9.1]
 
 ### Fixed

CONTRIBUTING.md

@@ -18,3 +18,16 @@ Requirements
 * Update the changelog
 * Run tests
 * Suggest version bump type
+
+## How to run tests and test coverage
+```bash
+>> pip install -r development-requirements.txt
+>> pytest reckoner/
+```
+
+With Coverage Reports
+```bash
+>> pytest --cov
+>> pytest --cov reckoner/ --cov-report=html #shows an html line coverage report in ./htmlcov/
+>> pytest --cov reckoner/ --cov-report=term #shows terminal coverage report of % coverage
+```

development-requirements.txt

@@ -0,0 +1,2 @@
+pytest
+pytest-cov

reckoner/__init__.py

@@ -1,86 +0,0 @@
-# -- coding: utf-8 --
-
-# Copyright 2017 Reactive Ops Inc.
-#
-# Licensed under the Apache License, Version 2.0 (the “License”);
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#    http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an “AS IS” BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.__init__.py
-
-import subprocess
-import logging
-
-from exception import ReckonerCommandException
-
-
-class Response(object):
-    """
-    Description:
-    - Utility class to simplify the results from call()
-
-    Arguments:
-    - stdout(string)
-    - stderr (string)
-    - exitcode (sting,int)
-
-    Attributes:
-    - stdout
-    - stderr
-    - exitcode
-
-    Returns:
-    - Instance of Response() is truthy where Reponse.exitcode == 0
-    - Instance Response() is falsey where Reponse.exitcode != 0
-    """
-
-    def __init__(self, stdout, stderr, exitcode):
-
-        self._dict = {}
-        self._dict['stdout'] = stdout
-        self._dict['stderr'] = stderr
-        self._dict['exitcode'] = exitcode
-
-    def __getattr__(self, name):
-        return self._dict.get(name)
-
-    def __str__(self):
-        return str(self._dict)
-
-    def __bool__(self):
-        return not self._dict['exitcode']
-
-    def __eq__(self, other):
-        return self._dict == other._dict
-
-
-def call(args, shell=False, executable=None):
-    """
-    Description:
-    - Wrapper for subprocess.Popen. Joins `args` and passes
-    to `subprocess.Popen`
-
-    Arguments: 
-    - args (list or string)
-
-    Returns:
-    - Instance of Response()
-    """
-    if type(args) == str:
-        args_string = args
-    else:
-        args_string = ' '.join(args)
-    logging.debug(args_string)
-    p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell, executable=executable)
-    stdout, stderr = p.communicate()
-    exitcode = p.returncode
-
-    if exitcode > 0:
-        raise ReckonerCommandException("Error with subprocess call: {}".format(args_string), stdout, stderr, exitcode)
-    return Response(stdout, stderr, exitcode)

reckoner/chart.py

@@ -24,8 +24,8 @@
 from exception import ReckonerCommandException
 from config import Config
 from repository import Repository
-from helm import Helm
-from . import call
+from helm.client import HelmClientException
+from command_line_caller import call
 
 default_repository = {'name': 'stable', 'url': 'https://kubernetes-charts.storage.googleapis.com'}
 
@@ -37,10 +37,10 @@ class Chart(object):
 
     Arguments:
     - chart (dict):
+    - helm: Instance of HelmClient()
 
     Attributes:
     - config: Instance of Config()
-    - helm: Instance of Helm()
     - release_name : String. Name of the release
     - name: String. Name of chart
     - files: List. Values files
@@ -51,12 +51,12 @@ class Chart(object):
     - Instance of Response() is falsey where Reponse.exitcode != 0
     """
 
-    def __init__(self, chart):
-        self.helm = Helm()
+    def __init__(self, chart, helm):
+        self.helm = helm
         self.config = Config()
         self._release_name = chart.keys()[0]
         self._chart = chart[self._release_name]
-        self._repository = Repository(self._chart.get('repository', default_repository))
+        self._repository = Repository(self._chart.get('repository', default_repository), self.helm)
         self._chart['values'] = self._chart.get('values', {})
 
         self._namespace = self._chart.get('namespace')
@@ -166,22 +166,22 @@ def update_dependencies(self):
         logging.debug("Updating chart dependencies: {}".format(self.chart_path))
         if os.path.exists(self.chart_path):
             try:
+                # TODO This is actually broken - not implemented (even before refactor)
                 r = self.helm.dependency_update(self.chart_path)
             except ReckonerCommandException, e:
-                logging.warn("Unable to update chart dependancies: {}".format(e.stderr))
+                logging.warn("Unable to update chart dependencies: {}".format(e.stderr))
 
     def install(self, namespace=None, context=None):
         """
         Description:
-        - Uprade --install the course chart
+        - Upgrade --install the course chart
 
         Arguments:
-        - namespace (string). Passed in but will be overriddne by Chart().namespace if set
+        - namespace (string). Passed in but will be overridden by Chart().namespace if set
 
         Returns:
         - Bool
         """
-        helm = Helm()
 
         # Set the namespace
         if self.namespace is None:
@@ -198,6 +198,7 @@ def install(self, namespace=None, context=None):
         # Update the helm dependencies
 
         if self.repository.git is None:
+            # TODO this is broken
             self.update_dependencies()
 
         # Build the args for the chart installation
@@ -220,15 +221,15 @@ def install(self, namespace=None, context=None):
 
         for key, value in self.values_strings.iteritems():
             for k, v in self._format_set(key, value):
-                self.args.append("--set-string={}={}".format(k, v))
+                self.args.append("--set={}={}".format(k, v))
 
         self.__check_env_vars()
+
         try:
-            r = helm.upgrade(self.args)
+            r = self.helm.upgrade(self.args)
             logging.info(r.stdout)
-
-        except ReckonerCommandException, e:
-            logging.error(e.stderr)
+        except HelmClientException, e:
+            logging.error(e)
             raise e
 
         self.post_install_hook()

reckoner/cli.py

@@ -19,7 +19,7 @@
 import coloredlogs
 import click
 import shutil
-from reckoner import Reckoner, call
+from reckoner import Reckoner
 import pkg_resources
 
 from meta import __version__

reckoner/command_line_caller/__init__.py

@@ -0,0 +1,82 @@
+# -- coding: utf-8 --
+
+# Copyright 2017 Reactive Ops Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the “License”);
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an “AS IS” BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.__init__.py
+
+import subprocess
+import logging
+
+
+class Response(object):
+    """
+    Description:
+    - Utility class to simplify the results from call()
+
+    Arguments:
+    - stdout(string)
+    - stderr (string)
+    - exitcode (sting,int)
+
+    Attributes:
+    - stdout
+    - stderr
+    - exitcode
+
+    Returns:
+    - Instance of Response() is truthy where Reponse.exitcode == 0
+    - Instance Response() is falsey where Reponse.exitcode != 0
+    """
+
+    def __init__(self, stdout, stderr, exitcode):
+
+        self._dict = {}
+        self._dict['stdout'] = stdout
+        self._dict['stderr'] = stderr
+        self._dict['exitcode'] = exitcode
+
+    def __getattr__(self, name):
+        return self._dict.get(name)
+
+    def __str__(self):
+        return str(self._dict)
+
+    def __bool__(self):
+        return not self._dict['exitcode']
+
+    def __eq__(self, other):
+        return self._dict == other._dict
+
+
+def call(args, shell=False, executable=None):
+    """
+    Description:
+    - Wrapper for subprocess.Popen. Joins `args` and passes
+    to `subprocess.Popen`
+
+    Arguments: 
+    - args (list or string)
+
+    Returns:
+    - Instance of Response()
+    """
+    if type(args) == str:
+        args_string = args
+    else:
+        args_string = ' '.join(args)
+    logging.debug(args_string)
+    p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=shell, executable=executable)
+    stdout, stderr = p.communicate()
+    exitcode = p.returncode
+
+    return Response(stdout, stderr, exitcode)

reckoner/config.py

@@ -19,7 +19,7 @@
 import sys
 import logging
 
-from . import call
+from command_line_caller import call
 from exception import ReckonerCommandException
 
 

reckoner/course.py

@@ -25,7 +25,7 @@
 from chart import Chart
 from repository import Repository
 from exception import MinimumVersionException, ReckonerCommandException
-from helm import Helm
+from helm.client import HelmClient, HelmClientException
 
 from meta import __version__ as reckoner_version
 
@@ -41,7 +41,7 @@ class Course(object):
 
     Attributes:
     - config: Instance of Config()
-    - helm: Instance of Helm()
+    - helm: Instance of HelmClient()
     - charts: List of Chart() instances
     - repositories: List of Repository() instances
 
@@ -52,16 +52,18 @@ def __init__(self, file):
         Parse course.yml contents into instances.
         """
         self.config = Config()
-        self.helm = Helm()
         self._dict = yaml.load(file)
+        if not self.config.helm_args:
+            self.config.helm_args = self._dict.get('helm_args')
+        self.helm = HelmClient(default_helm_arguments=self.config.helm_args)
         self._repositories = []
         self._charts = []
         for name, repository in self._dict.get('repositories', {}).iteritems():
             repository['name'] = name
-            self._repositories.append(Repository(repository))
+            self._repositories.append(Repository(repository, self.helm))
 
         for name, chart in self._dict.get('charts', {}).iteritems():
-            self._charts.append(Chart({name: chart}))
+            self._charts.append(Chart({name: chart}, self.helm))
 
         for repo in self._repositories:
             type(repo)
@@ -74,9 +76,6 @@ def __init__(self, file):
         if not self.config.local_development:
             self._compare_required_versions()
 
-        if not self.config.helm_args:
-            self.config.helm_args = self._dict.get('helm_args')
-
     def __str__(self):
         return str(self._dict)