From aa75c3eb4578600d468cfc80dc5023d75471075e Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Mon, 9 Sep 2019 11:48:05 -0500 Subject: [PATCH] fix: Align template file naming with Helm CLI Armada has previously named template files relative to the `templates` dir, whereas the Helm CLI names them relative to the chart root. This causes `include`s of these templates to fail. This change fixes this, for armada/Chart/v2 docs only, since it is a breaking change, as some charts may have already aligned with the existing Armada behavior. When updating a release previously deployed with armada/Chart/v1, the fixed template names alone will not cause the release to be updated, as the diff logic accounts for this. Change-Id: I243073ca4c2e1edbcb0d8f649475f568fc7c818f --- armada/handlers/chartbuilder.py | 51 ++++++++++++++----- armada/handlers/release_diff.py | 14 ++++- .../tests/unit/handlers/test_chartbuilder.py | 3 ++ .../operations/documents/migration-v1-v2.rst | 9 ++++ 4 files changed, 64 insertions(+), 13 deletions(-) diff --git a/armada/handlers/chartbuilder.py b/armada/handlers/chartbuilder.py index cb1f42fe..35a9a9ee 100644 --- a/armada/handlers/chartbuilder.py +++ b/armada/handlers/chartbuilder.py @@ -24,8 +24,9 @@ from oslo_config import cfg from oslo_log import log as logging import yaml -from armada.exceptions import chartbuilder_exceptions from armada import const +from armada.exceptions import chartbuilder_exceptions +from armada.handlers.schema import get_schema_info LOG = logging.getLogger(__name__) @@ -51,18 +52,31 @@ class ChartBuilder(object): source_dir = chart_data.get('source_dir') source_directory = os.path.join(*source_dir) dependencies = chart_data.get('dependencies') + + # TODO: Remove when v1 doc support is removed. + schema_info = get_schema_info(chart['schema']) + if schema_info.version < 2: + fix_tpl_name = False + else: + fix_tpl_name = True + if dependencies is not None: dependency_builders = [] for chart_dep in dependencies: builder = ChartBuilder.from_chart_doc(chart_dep) dependency_builders.append(builder) - return cls(name, source_directory, dependency_builders) + return cls( + name, + source_directory, + dependency_builders, + fix_tpl_name=fix_tpl_name) - return cls.from_source(name, source_directory) + return cls.from_source( + name, source_directory, fix_tpl_name=fix_tpl_name) @classmethod - def from_source(cls, name, source_directory): + def from_source(cls, name, source_directory, fix_tpl_name=False): ''' Returns a ChartBuilder, which gets it dependencies from within the Helm chart itself. @@ -84,12 +98,19 @@ class ChartBuilder(object): if re.match(r'^[._]', f.name): continue - builder = ChartBuilder.from_source(f.name, f.path) + builder = ChartBuilder.from_source( + f.name, f.path, fix_tpl_name=fix_tpl_name) dependency_builders.append(builder) - return cls(name, source_directory, dependency_builders) + return cls( + name, + source_directory, + dependency_builders, + fix_tpl_name=fix_tpl_name) - def __init__(self, name, source_directory, dependency_builders): + def __init__( + self, name, source_directory, dependency_builders, + fix_tpl_name=False): ''' :param name: A name to use for the chart. :param source_directory: The source directory of the Helm chart. @@ -99,6 +120,7 @@ class ChartBuilder(object): self.name = name self.source_directory = source_directory self.dependency_builders = dependency_builders + self.fix_tpl_name = fix_tpl_name # cache for generated protoc chart object self._helm_chart = None @@ -248,17 +270,22 @@ class ChartBuilder(object): ''' chart_name = self.name templates = [] - if not os.path.exists(os.path.join(self.source_directory, - 'templates')): + tpl_dir = os.path.join(self.source_directory, 'templates') + if not os.path.exists(tpl_dir): LOG.warn( "Chart %s has no templates directory. " "No templates will be deployed", chart_name) - for root, _, files in os.walk(os.path.join(self.source_directory, - 'templates'), topdown=True): + for root, _, files in os.walk(tpl_dir, topdown=True): for tpl_file in files: tname = os.path.relpath( os.path.join(root, tpl_file), - os.path.join(self.source_directory, 'templates')) + # For v1 compatibility, name template relative to template + # dir, for v2 fix the name to be relative to the chart root + # to match Helm CLI behavior. + self.source_directory if self.fix_tpl_name else tpl_dir) + # NOTE: If the template name is fixed (see above), then this + # also fixes the path passed here, which could theoretically + # affect which files get ignored, though unlikely. if self.ignore_file(tname): LOG.debug('Ignoring file %s', tname) continue diff --git a/armada/handlers/release_diff.py b/armada/handlers/release_diff.py index d80c3bb4..7c5321dd 100644 --- a/armada/handlers/release_diff.py +++ b/armada/handlers/release_diff.py @@ -77,7 +77,19 @@ class ReleaseDiff(object): chart_desc = '{} ({})'.format(chart.metadata.name, desc) raise armada_exceptions.InvalidValuesYamlException(chart_desc) files = {f.type_url: f.value for f in chart.files} - templates = {t.name: t.data for t in chart.templates} + + # With armada/Chart/v1, Armada deployed releases with incorrect + # template names, omitting the `templates/` prefix, which is fixed in + # v2. This aligns these template names, so that the prefixes match, to + # avoid unwanted updates to releases when consuming this fix. + def fix_tpl_name(tpl_name): + CORRECT_PREFIX = 'templates/' + if tpl_name.startswith(CORRECT_PREFIX): + return tpl_name + return CORRECT_PREFIX + tpl_name + + templates = {fix_tpl_name(t.name): t.data for t in chart.templates} + dependencies = { d.metadata.name: self.make_chart_dict( d, '{}({} dependency)'.format(desc, d.metadata.name)) diff --git a/armada/tests/unit/handlers/test_chartbuilder.py b/armada/tests/unit/handlers/test_chartbuilder.py index 70b0d832..de202c44 100644 --- a/armada/tests/unit/handlers/test_chartbuilder.py +++ b/armada/tests/unit/handlers/test_chartbuilder.py @@ -60,6 +60,7 @@ class BaseChartBuilderTestCase(testtools.TestCase): """ chart_stream = """ + schema: armada/Chart/v1 metadata: name: test data: @@ -90,6 +91,7 @@ class BaseChartBuilderTestCase(testtools.TestCase): """ dependency_chart_stream = """ + schema: armada/Chart/v1 metadata: name: dep data: @@ -127,6 +129,7 @@ class BaseChartBuilderTestCase(testtools.TestCase): def _get_test_chart(self, chart_dir): return { + 'schema': 'armada/Chart/v1', 'metadata': { 'name': 'test' }, diff --git a/doc/source/operations/documents/migration-v1-v2.rst b/doc/source/operations/documents/migration-v1-v2.rst index 3b796552..c36bea37 100644 --- a/doc/source/operations/documents/migration-v1-v2.rst +++ b/doc/source/operations/documents/migration-v1-v2.rst @@ -56,6 +56,15 @@ Chart | now optional, deafults to no | | | subpath. | | +--------------------------------+------------------------------------------------------------+ +| Template naming for template | If a chart was relying on Armada's previous misaligned | +| files aligned with Helm CLI. | template naming, where it was omitting the ``templates/`` | +| | prefix, such as via ``include`` argument, that argument | +| | will need to be updated. This could also theoretically | +| | affect whether the file is ignored, if the old or new | +| | name is in ``.helmignore`` (unlikely). The fixed template | +| | names alone will not cause a release to be updated, as the | +| | diff logic accounts for this. | ++--------------------------------+------------------------------------------------------------+ | ``wait`` improvements | See `Wait Improvements`_. | +--------------------------------+------------------------------------------------------------+