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

@@ -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 }}

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).

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
 
 

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"

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

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

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'"
         )