Fixes for repository/repositories settings with git (#154)

* Adjustments for repositories in course.yml

- Removed the default of None in repository 'install' since it's used throughout the method
  and should be a required parameter
- Adjusted the repo install method to include a name and version when called from course.py
  so that it does not fail when trying to install a git repo in the root definition of repositories

* Added note that this section does nothing discernable

* Adjustments

- Adjusted course to only install helm repos if they are NOT git ones (from the course layer)
- Added a find and replace to rationalize the string reference of a repository in a chart with a repository from the course.yml list (to avoid strange issues with git)
- Added a test, although it tests the implementation of rationalizing
  the repo settings

* Adjusted git support testing to extend different configuration setups

* Added fix to changelog

CHANGELOG.md

@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fixed issues where helm would try to interpolate variables in comments provided in `values: {}` section of a chart definition
 - Added better logging for debug when unexpected errors happen in python
 - Added better logging output for debugging yaml parse issues
+- Fixed repository git support for defining git chart endpoints at the top level and chart level
 
 ## [2.1.1]
 ### Fixes

end_to_end_testing/run_end_to_end.sh

@@ -239,7 +239,7 @@ function e2e_test_git_chart() {
 
     # Special Clean up of GoHarbor
     helm delete --purge go-harbor
-    kubectl delete pvc -n test --all
+    kubectl delete pvc --all-namespaces --all
 }
 
 function e2e_test_stop_after_first_failure() {

end_to_end_testing/test_git_chart.yml

@@ -8,21 +8,34 @@ repositories:
     url: https://charts.fairwinds.com/stable
   fairwinds-incubator:
     url: https://charts.fairwinds.com/incubator
+  polaris-repo-test:
+    git: https://github.com/FairwindsOps/charts
+    path: stable
+  the-go-harbor-repo:
+    git: https://github.com/goharbor/harbor-helm.git
 minimum_versions: #set minimum version requirements here
   helm: 0.0.0
   reckoner: 0.0.0
 charts:
   polaris-release:
     namespace: polaris
     chart: polaris
-    repository:
-      git: https://github.com/FairwindsOps/charts
-      path: stable
+    repository: polaris-repo-test
   polaris:
     namespace: another-polaris
     repository:
       git: https://github.com/FairwindsOps/charts.git
       path: stable
   go-harbor:
+    chart: go-harbor
     repository:
       git: https://github.com/goharbor/harbor-helm.git
+    values:
+      persistence:
+        enabled: false
+  go-harbor-two:
+    namespace: a-different-one
+    repository: the-go-harbor-repo
+    values:
+      persistence:
+        enabled: false

reckoner/course.py

@@ -71,12 +71,16 @@ def __init__(self, course_file: BufferedReader):
             self._repositories.append(Repository(repository, self.helm))
 
         for name, chart in self._dict.get('charts', {}).items():
+            self._set_chart_repository(chart)
             self._charts.append(Chart({name: chart}, self.helm))
 
         for repo in self._repositories:
-            type(repo)
-            logging.debug("Installing repository: {}".format(repo))
-            repo.install()
+            # Skip install of repo if it is git based since it will be installed from the chart class
+            if repo.git is None:
+                logging.debug("Installing repository: {}".format(repo))
+                repo.install(chart_name=repo._repository['name'], version=repo._repository.get('version'))
+            else:
+                logging.debug("Skipping git install of repository to later be installed at the chart level: {}".format(repo))
 
         self.helm.repo_update()
 
@@ -86,6 +90,25 @@ def __init__(self, course_file: BufferedReader):
             logging.error(e)
             sys.exit(1)
 
+    # HACK: This logic is here to try to replace a named reference to a helm repo defined in the main
+    #       course repositories. This is because we want to pass the git repo and some
+    #       other data to the chart (since the chart also tries to install the repo)
+    #
+    # NOTE: The real fix here would be to unify the way repositories are installed and managed instead
+    #       running the install() function twice from the course.yml and from the charts.yml.
+    #
+    # IF: chart has a repository definition that is a string reference, then find that
+    # reference (if exists) from the main repositories for the course and replace the
+    # string definition with the repositories setting.
+    def _set_chart_repository(self, chart: dict):
+        """_set_chart_repository will convert the string reference of a
+        repository into the dictionary configuration of that repository
+        or, if None, or if the string isn't in the repositories section,
+        it will leave it alone."""
+        if isinstance(chart.get('repository', None), str) and chart['repository'] in [x.name for x in self.repositories]:
+            logging.debug('Found a reference to a repository installed via repositories section of course, replacing reference.')
+            chart['repository'] = self._dict['repositories'][chart['repository']]
+
     def __str__(self):
         return str(self._dict)
 

reckoner/repository.py

@@ -38,7 +38,7 @@ class Repository(object):
     """
 
     def __init__(self, repository, helm_client: HelmClient):
-        super(type(self), self).__init__()
+        super(type(self), self).__init__()  # TODO: Remove this and test thoroughly, it appears this doesn't do anything.
         self.config = Config()
         self._repository = {}
         self._helm_client = helm_client
@@ -67,7 +67,7 @@ def __eq__(self, other):
     def chart_path(self):
         return self._chart_path
 
-    def install(self, chart_name=None, version=None):
+    def install(self, chart_name, version=None):
         """ Install Helm repository """
         # TODO: This function needs some love - seems like it wants to return T/F and maybe have logic act on that vs raising errors when you cannot install (from this function)
         if self.git is None:

reckoner/tests/test_course.py

@@ -174,4 +174,52 @@ def test_course_raises_errors_on_bad_client_response(self, mockGetHelm, mockYAML
         with self.assertRaises(ReckonerException):
             Course(None)
 
+    # This test is intended to test the implementation that repository field in charts is resolved to the repositories settings in course.yml; because the repositories code is brittle.
+    # This is important because if the chart references a git repo from the main course repositories, it breaks the repo install method
+    def test_course_git_repository_handling(self, mockGetHelm, mockYAML):
+        """Assure that course replaces strings with object settings for chart repository settings"""
+        course = mockYAML.load.return_value = self.course_yaml
+        # Add repositories configuration to the course
+        course['repositories'] = {
+            'some-git-repo': {
+                'git': 'https://git.com/my-chart.git',
+                'path': 'charts',
+            },
+            'stablez': {
+                'url': 'https://fake.url.com',
+            }
+        }
+        # Add first chart reference to the repositories settings
+        course['charts']['first-chart']['repository'] = 'some-git-repo'
+        # Add second chart referencing a missing chart (current behavior is to keep the string)
+        course['charts']['second-chart'] = {
+            'repository': 'missingfromrepos',
+            'chart': 'my-chart',
+        }
+        # Add a third chart referring to a non-git repo
+        course['charts']['third-chart'] = {
+            'chart': 'my-chart',
+            'repository': 'stablez',
+        }
+
+        self.assertIsInstance(course['charts']['first-chart']['repository'], str)
+
+        # Mock out the repository helm client
+        with mock.patch('reckoner.repository.HelmClient', mockGetHelm):
+            # Run the course to convert the chart['first-chart']['repository'] reference
+            Course(None)
+
+        # Assert the chart repo setting went from string to dict
+        self.assertIsInstance(course['charts']['first-chart']['repository'], dict)
+
+        # Assert that the dict for the repositories settings is the same as the charts repository setting after course loads
+        self.assertDictEqual(course['charts']['first-chart']['repository'], course['repositories']['some-git-repo'])
+
+        # Assert that second-chart is left alone since it is not in repositories
+        self.assertEqual(course['charts']['second-chart']['repository'], 'missingfromrepos')
+
+        # Assert that third-chart is reconciled to settings from repositories block
+        self.assertDictEqual(course['charts']['third-chart']['repository'], course['repositories']['stablez'])
+
+
 # TODO: Add test for calling plot against a chart that doesn't exist in your course.yml