From b5a9f498e1654ae202b1f1d66e8345420931f3d0 Mon Sep 17 00:00:00 2001 From: Nick Huanca Date: Fri, 8 Nov 2019 17:20:51 +0100 Subject: [PATCH] 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 | 1 + end_to_end_testing/run_end_to_end.sh | 2 +- end_to_end_testing/test_git_chart.yml | 19 +++++++++-- reckoner/course.py | 29 ++++++++++++++-- reckoner/repository.py | 4 +-- reckoner/tests/test_course.py | 48 +++++++++++++++++++++++++++ 6 files changed, 94 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a14042c5..26dde276 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/end_to_end_testing/run_end_to_end.sh b/end_to_end_testing/run_end_to_end.sh index 37ecd20c..01d47719 100755 --- a/end_to_end_testing/run_end_to_end.sh +++ b/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() { diff --git a/end_to_end_testing/test_git_chart.yml b/end_to_end_testing/test_git_chart.yml index d617c75b..a6004c0a 100644 --- a/end_to_end_testing/test_git_chart.yml +++ b/end_to_end_testing/test_git_chart.yml @@ -8,6 +8,11 @@ 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 @@ -15,14 +20,22 @@ 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 diff --git a/reckoner/course.py b/reckoner/course.py index ac47c883..72b63c06 100644 --- a/reckoner/course.py +++ b/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) diff --git a/reckoner/repository.py b/reckoner/repository.py index a6aad6c1..a086ffc3 100644 --- a/reckoner/repository.py +++ b/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: diff --git a/reckoner/tests/test_course.py b/reckoner/tests/test_course.py index d1d1af24..fa7cdc8a 100644 --- a/reckoner/tests/test_course.py +++ b/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