From b44fdd3f0500064d37f1858bb05d6297a3d844ef Mon Sep 17 00:00:00 2001 From: Bikouo Aubin <79859644+abikouo@users.noreply.github.com> Date: Mon, 13 Nov 2023 11:48:13 +0100 Subject: [PATCH] helm - fix issue for helm command when chart contains space into its name (#657) * fix issue for helm command when chart contains space into its name --- .github/workflows/integration-tests.yaml | 47 ++++++++------- .../fragments/20231110-helm-quote-ref.yaml | 3 + plugins/modules/helm.py | 6 +- .../targets/helm/defaults/main.yml | 1 + .../targets/helm/tasks/run_test.yml | 3 + .../test_helm_with_space_into_chart_name.yml | 58 +++++++++++++++++++ tests/unit/modules/test_module_helm.py | 40 ++++++------- 7 files changed, 113 insertions(+), 45 deletions(-) create mode 100644 changelogs/fragments/20231110-helm-quote-ref.yaml create mode 100644 tests/integration/targets/helm/tasks/test_helm_with_space_into_chart_name.yml diff --git a/.github/workflows/integration-tests.yaml b/.github/workflows/integration-tests.yaml index 964f2252..03b89acd 100644 --- a/.github/workflows/integration-tests.yaml +++ b/.github/workflows/integration-tests.yaml @@ -17,7 +17,9 @@ jobs: source_dir: "./source" runs-on: ubuntu-latest outputs: - test_targets: ${{ steps.display.outputs.test_targets }} + test_targets: ${{ steps.splitter.outputs.test_targets }} + test_targets_json: ${{ steps.splitter.outputs.test_targets_json }} + test_jobs: ${{ steps.splitter.outputs.test_jobs }} steps: - name: Checkout the collection repository uses: actions/checkout@v3 @@ -32,12 +34,15 @@ jobs: collections_to_test: ${{ env.source_dir }} total_jobs: 8 - - name: display targets - id: display - run: echo "test_targets=${{ steps.splitter.outputs.test_targets }}" >> $GITHUB_OUTPUT + - name: Display splitter output + run: | + echo "test_targets=${{ steps.splitter.outputs.test_targets }}" + echo "test_targets_json=${{ steps.splitter.outputs.test_targets_json }}" + echo "test_jobs=${{ steps.splitter.outputs.test_jobs }}" shell: bash - integration: + runs-on: ubuntu-latest + timeout-minutes: 60 needs: - splitter if: ${{ needs.splitter.outputs.test_targets != '' }} @@ -45,9 +50,6 @@ jobs: source: "./source" cloud_common: "./cloudcommon" ansible_posix: "./ansible_posix" - test_targets: ${{ needs.splitter.outputs.test_targets }} - runs-on: ubuntu-latest - timeout-minutes: 60 strategy: fail-fast: false matrix: @@ -58,22 +60,23 @@ jobs: enable-turbo-mode: - true - false - job-index: [1, 2, 3, 4, 5, 6, 7, 8] - name: "integration-py${{ matrix.python-version }}-${{ matrix.ansible-version }}-turbo-mode=${{ matrix.enable-turbo-mode }}-${{ matrix.job-index }}" + workflow-id: ${{ fromJson(needs.splitter.outputs.test_jobs) }} + name: "integration-py${{ matrix.python-version }}-${{ matrix.ansible-version }}-${{ matrix.workflow-id }}" steps: - - name: Read ansible-test targets + - name: Read target id: read-targets - run: >- - echo "ansible_test_targets=$(echo "${{ env.test_targets }}" | sed s/';'/'\n'/g | - grep "kubernetes.core-${{ matrix.job-index }}" | cut -d ':' -f2 | sed s/','/' '/g)" >> $GITHUB_OUTPUT - shell: bash - - - name: Display targets - run: >- - echo "targets to test: $ANSIBLE_TARGETS" - shell: bash + run: | + import json, os + with open(os.environ.get('GITHUB_OUTPUT'), "a", encoding="utf-8") as fh: + fh.write(f'ansible_test_targets={json.loads(os.environ.get("ALL_TEST_TARGETS")).get(os.environ.get("WORKFLOW_ID"))}\n') + shell: python env: - ANSIBLE_TARGETS: ${{ steps.read-targets.outputs.ansible_test_targets }} + ALL_TEST_TARGETS: ${{ needs.splitter.outputs.test_targets_json }} + WORKFLOW_ID: ${{ matrix.workflow-id }} + + - name: Display ansible test targets + run: | + echo "ansible_test_targets -> ${{ steps.read-targets.outputs.ansible_test_targets }}" - name: Checkout kubernetes.core repository uses: actions/checkout@v3 @@ -81,7 +84,7 @@ jobs: path: ${{ env.source }} ref: ${{ github.event.pull_request.head.sha }} - - name: Set up Python ${{ env.python-version }} + - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v4 with: python-version: ${{ matrix.python-version }} diff --git a/changelogs/fragments/20231110-helm-quote-ref.yaml b/changelogs/fragments/20231110-helm-quote-ref.yaml new file mode 100644 index 00000000..fd8af269 --- /dev/null +++ b/changelogs/fragments/20231110-helm-quote-ref.yaml @@ -0,0 +1,3 @@ +--- +bugfixes: + - helm - Put the chart_ref into quotes when running ``helm show chart``, ``helm upgrade`` and ``helm dependency update`` commands (https://github.com/ansible-collections/kubernetes.core/issues/653). diff --git a/plugins/modules/helm.py b/plugins/modules/helm.py index dc756e72..2a29d027 100644 --- a/plugins/modules/helm.py +++ b/plugins/modules/helm.py @@ -472,7 +472,7 @@ def run_dep_update(module, chart_ref): """ Run dependency update """ - dep_update = module.get_helm_binary() + " dependency update " + chart_ref + dep_update = module.get_helm_binary() + f" dependency update '{chart_ref}'" rc, out, err = module.run_helm_command(dep_update) @@ -480,7 +480,7 @@ def fetch_chart_info(module, command, chart_ref): """ Get chart info """ - inspect_command = command + " show chart " + chart_ref + inspect_command = command + f" show chart '{chart_ref}'" rc, out, err = module.run_helm_command(inspect_command) @@ -572,7 +572,7 @@ def deploy( if set_value_args: deploy_command += " " + set_value_args - deploy_command += " " + release_name + " " + chart_name + deploy_command += " " + release_name + f" '{chart_name}'" return deploy_command diff --git a/tests/integration/targets/helm/defaults/main.yml b/tests/integration/targets/helm/defaults/main.yml index 4c9331c9..39841cce 100644 --- a/tests/integration/targets/helm/defaults/main.yml +++ b/tests/integration/targets/helm/defaults/main.yml @@ -25,3 +25,4 @@ test_namespace: - "helm-from-repository" - "helm-from-url" - "helm-reuse-values" + - "helm-chart-with-space-into-name" diff --git a/tests/integration/targets/helm/tasks/run_test.yml b/tests/integration/targets/helm/tasks/run_test.yml index 82457cde..76d55b22 100644 --- a/tests/integration/targets/helm/tasks/run_test.yml +++ b/tests/integration/targets/helm/tasks/run_test.yml @@ -34,6 +34,9 @@ - name: Test helm uninstall include_tasks: test_helm_uninstall.yml +- name: Test helm install with chart name containing space + include_tasks: test_helm_with_space_into_chart_name.yml + # https://github.com/ansible-collections/community.kubernetes/issues/296 - name: Test Skip CRDS feature in helm chart install include_tasks: test_crds.yml diff --git a/tests/integration/targets/helm/tasks/test_helm_with_space_into_chart_name.yml b/tests/integration/targets/helm/tasks/test_helm_with_space_into_chart_name.yml new file mode 100644 index 00000000..fcf86766 --- /dev/null +++ b/tests/integration/targets/helm/tasks/test_helm_with_space_into_chart_name.yml @@ -0,0 +1,58 @@ +--- +- name: Create test directory + ansible.builtin.tempfile: + state: directory + suffix: .helm + register: test_dir + +- name: Test helm using directory with space + vars: + helm_dir: "{{ test_dir.path }}/Deploy Chart" + helm_namespace: "{{ test_namespace[10] }}" + chart_release_name: "deploy-chart-with-space-into-name" + helm_local_src: "test-chart" + block: + - name: Copy helm file into destination + ansible.builtin.copy: + src: "{{ helm_local_src }}" + dest: "{{ helm_dir }}" + + - name: Install chart from local source with Space into name + helm: + binary_path: "{{ helm_binary }}" + name: "{{ chart_release_name }}" + chart_ref: "{{ helm_dir }}/{{ helm_local_src | basename }}" + namespace: "{{ helm_namespace }}" + create_namespace: true + register: install_chart + + - name: Assert that chart is installed + assert: + that: + - install_chart is changed + - install_chart.status.status | lower == 'deployed' + + - name: Check helm_info content + helm_info: + binary_path: "{{ helm_binary }}" + name: "{{ chart_release_name }}" + namespace: "{{ helm_namespace }}" + register: chart_info + + - name: Assert that Chart is installed (using helm_info) + assert: + that: + - chart_info.status.status | lower == 'deployed' + + always: + - name: Delete temporary directory + ansible.builtin.file: + state: absent + name: "{{ test_dir.path }}" + + - name: Remove helm namespace + k8s: + api_version: v1 + kind: Namespace + name: "{{ helm_namespace }}" + state: absent diff --git a/tests/unit/modules/test_module_helm.py b/tests/unit/modules/test_module_helm.py index 50ce8a4f..199bd828 100644 --- a/tests/unit/modules/test_module_helm.py +++ b/tests/unit/modules/test_module_helm.py @@ -86,12 +86,12 @@ class TestDependencyUpdateWithoutChartRepoUrlOption(unittest.TestCase): helm.main() helm.run_dep_update.assert_not_called() mock_run_command.assert_called_once_with( - "/usr/bin/helm upgrade -i --reset-values test /tmp/path", + "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test /tmp/path" + == "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'" ) def test_dependency_update_option_false(self): @@ -116,12 +116,12 @@ class TestDependencyUpdateWithoutChartRepoUrlOption(unittest.TestCase): helm.main() helm.run_dep_update.assert_not_called() mock_run_command.assert_called_once_with( - "/usr/bin/helm upgrade -i --reset-values test /tmp/path", + "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test /tmp/path" + == "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'" ) def test_dependency_update_option_true(self): @@ -145,14 +145,14 @@ class TestDependencyUpdateWithoutChartRepoUrlOption(unittest.TestCase): mock_run_command.assert_has_calls( [ call( - "/usr/bin/helm upgrade -i --reset-values test /tmp/path", + "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'", environ_update={"HELM_NAMESPACE": "test"}, ) ] ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test /tmp/path" + == "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'" ) def test_dependency_update_option_true_without_dependencies_block(self): @@ -179,14 +179,14 @@ class TestDependencyUpdateWithoutChartRepoUrlOption(unittest.TestCase): mock_run_command.assert_has_calls( [ call( - "/usr/bin/helm upgrade -i --reset-values test /tmp/path", + "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'", environ_update={"HELM_NAMESPACE": "test"}, ) ] ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test /tmp/path" + == "/usr/bin/helm upgrade -i --reset-values test '/tmp/path'" ) @@ -249,12 +249,12 @@ class TestDependencyUpdateWithChartRepoUrlOption(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test chart1", + "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test 'chart1'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test chart1" + == "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test 'chart1'" ) def test_dependency_update_option_False(self): @@ -278,12 +278,12 @@ class TestDependencyUpdateWithChartRepoUrlOption(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test chart1", + "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test 'chart1'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test chart1" + == "/usr/bin/helm --repo=http://repo.example/charts upgrade -i --reset-values test 'chart1'" ) def test_dependency_update_option_True_and_replace_option_disabled(self): @@ -336,12 +336,12 @@ class TestDependencyUpdateWithChartRepoUrlOption(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm --repo=http://repo.example/charts install --dependency-update --replace test chart1", + "/usr/bin/helm --repo=http://repo.example/charts install --dependency-update --replace test 'chart1'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm --repo=http://repo.example/charts install --dependency-update --replace test chart1" + == "/usr/bin/helm --repo=http://repo.example/charts install --dependency-update --replace test 'chart1'" ) @@ -403,12 +403,12 @@ class TestDependencyUpdateWithChartRefIsUrl(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm upgrade -i --reset-values test http://repo.example/charts/application.tgz", + "/usr/bin/helm upgrade -i --reset-values test 'http://repo.example/charts/application.tgz'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test http://repo.example/charts/application.tgz" + == "/usr/bin/helm upgrade -i --reset-values test 'http://repo.example/charts/application.tgz'" ) def test_dependency_update_option_False(self): @@ -431,12 +431,12 @@ class TestDependencyUpdateWithChartRefIsUrl(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm upgrade -i --reset-values test http://repo.example/charts/application.tgz", + "/usr/bin/helm upgrade -i --reset-values test 'http://repo.example/charts/application.tgz'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm upgrade -i --reset-values test http://repo.example/charts/application.tgz" + == "/usr/bin/helm upgrade -i --reset-values test 'http://repo.example/charts/application.tgz'" ) def test_dependency_update_option_True_and_replace_option_disabled(self): @@ -487,10 +487,10 @@ class TestDependencyUpdateWithChartRefIsUrl(unittest.TestCase): with self.assertRaises(AnsibleExitJson) as result: helm.main() mock_run_command.assert_called_once_with( - "/usr/bin/helm install --dependency-update --replace test http://repo.example/charts/application.tgz", + "/usr/bin/helm install --dependency-update --replace test 'http://repo.example/charts/application.tgz'", environ_update={"HELM_NAMESPACE": "test"}, ) assert ( result.exception.args[0]["command"] - == "/usr/bin/helm install --dependency-update --replace test http://repo.example/charts/application.tgz" + == "/usr/bin/helm install --dependency-update --replace test 'http://repo.example/charts/application.tgz'" )