fix(argo): clear stale child overrides + flip --commit to opt-in

src/edge_containers_cli/cli.py

@@ -304,7 +304,9 @@ async def start(
         autocompletion=all_svc,
         show_default=False,
     ),
-    commit: bool = typer.Option(True, help="Commits the values to the git repo"),
+    commit: bool = typer.Option(
+        False, help="Also commit the change to the git repo for an audit trail"
+    ),
 ):
     """Start a service"""
     try:
@@ -323,7 +325,9 @@ async def stop(
         autocompletion=running_svc,
         show_default=False,
     ),
-    commit: bool = typer.Option(True, help="Commits the values to the git repo"),
+    commit: bool = typer.Option(
+        False, help="Also commit the change to the git repo for an audit trail"
+    ),
 ):
     """Stop a service"""
     try:

src/edge_containers_cli/cmds/argo_commands.py

@@ -75,6 +75,16 @@ async def patch_value(target: str, key: str, value: YamlTypes):
     # Rely on argocd autosync to get the cluster into the right state
 
 
+async def _unset_key_and_children(target: str, key: str):
+    cmd_unset = f"argocd app unset {target} -p {key}"
+    await shell.run_command(cmd_unset, skip_on_dryrun=True)
+    app_patches = await get_patches(target)
+    for patch in app_patches:
+        if re.match(rf"{key}\..*", patch["name"]):
+            cmd_unset_child = f"argocd app unset {target} -p {patch['name']}"
+            await shell.run_command(cmd_unset_child, skip_on_dryrun=True)
+
+
 @do_retry
 async def push_value(target: str, key: str, value: YamlTypes):
     # Get source details
@@ -87,9 +97,8 @@ async def push_value(target: str, key: str, value: YamlTypes):
 
     await set_value(repo_url, path / "values.yaml", key, value)
 
-    # Free a possible patched value & refresh repo
-    cmd_unset = f"argocd app unset {target} -p {key}"
-    await shell.run_command(cmd_unset, skip_on_dryrun=True)
+    # Free a possible patched value, its children & refresh repo
+    await _unset_key_and_children(target, key)
     cmd_refresh = f"argocd app get {target} --refresh"
     await shell.run_command(cmd_refresh, skip_on_dryrun=True)
     # Rely on argocd autosync to get the cluster into the right state
@@ -108,13 +117,7 @@ async def push_remove_key(target: str, key: str):
     await del_key(repo_url, path / "values.yaml", key)
 
     # Free a possible patched value, its children & refresh repo
-    cmd_unset = f"argocd app unset {target} -p {key}"
-    await shell.run_command(cmd_unset, skip_on_dryrun=True)
-    app_patches = await get_patches(target)
-    for patch in app_patches:
-        if re.match(rf"{key}\..*", patch["name"]):
-            cmd_unset_child = f"argocd app unset {target} -p {patch['name']}"
-            await shell.run_command(cmd_unset_child, skip_on_dryrun=True)
+    await _unset_key_and_children(target, key)
     cmd_refresh = f"argocd app get {target} --refresh"
     await shell.run_command(cmd_refresh, skip_on_dryrun=True)
     # Rely on argocd autosync to get the cluster into the right state
@@ -233,14 +236,14 @@ async def restart(self, service_name):
         cmd = f"argocd app delete-resource {namespace}/{service_name} --kind StatefulSet --all"
         await shell.run_command(cmd, skip_on_dryrun=True)
 
-    async def start(self, service_name, commit=True):
+    async def start(self, service_name, commit=False):
         await self._check_stoppable(service_name)
         if commit:
             await push_value(self.target, f"services.{service_name}.enabled", True)
         else:
             await patch_value(self.target, f"services.{service_name}.enabled", True)
 
-    async def stop(self, service_name, commit=True):
+    async def stop(self, service_name, commit=False):
         await self._check_stoppable(service_name)
         if commit:
             await push_value(self.target, f"services.{service_name}.enabled", False)

tests/data/argocd.yaml

@@ -113,6 +113,8 @@ deploy:
     rsp: ""
   - cmd: argocd app unset namespace/bl01t -p services.bl01t-ea-test-01
     rsp: ""
+  - cmd: argocd app get --show-params namespace/bl01t -o json
+    rsp: "{}"
   - cmd: argocd app get namespace/bl01t --refresh
     rsp: ""
 
@@ -163,6 +165,8 @@ start_commit:
     rsp: ""
   - cmd: argocd app unset namespace/bl01t -p services.bl01t-ea-test-01.enabled
     rsp: ""
+  - cmd: argocd app get --show-params namespace/bl01t -o json
+    rsp: "{}"
   - cmd: argocd app get namespace/bl01t --refresh
     rsp: ""
 
@@ -199,6 +203,8 @@ stop_commit:
     rsp: ""
   - cmd: argocd app unset namespace/bl01t -p services.bl01t-ea-test-01.enabled
     rsp: ""
+  - cmd: argocd app get --show-params namespace/bl01t -o json
+    rsp: "{}"
   - cmd: argocd app get namespace/bl01t --refresh
     rsp: ""
 
@@ -214,3 +220,85 @@ stop:
           enabled: true
   - cmd: argocd app set namespace/bl01t -p services.bl01t-ea-test-01.enabled=False
     rsp: ""
+
+deploy_clears_override:
+  # Same flow as `deploy`, but the parent app has a lingering
+  # `services.bl01t-ea-test-01.enabled=false` parameter override (e.g. from
+  # an earlier `ec stop --no-commit` or Monitor's stop button). The fix
+  # makes push_value also unset child overrides, so the redeployed service
+  # actually starts.
+  - cmd: git clone https://github.com/epics-containers/bl01t-services /tmp/.*
+    rsp: Cloning into /tmp/xxxx...
+  - cmd: git tag --sort=committerdate
+    rsp: |
+      1.0
+      2.0
+  - cmd: git ls-tree -r 1.0 --name-only
+    rsp: |
+      services/bl01t-ea-test-01
+  - cmd: git ls-tree -r 1.0
+    rsp: |
+      100644 blob b7b39845b55fb4d45d58ba86ef4527917877d556    services/bl01t-ea-test-01/Chart.yaml
+  - cmd: git diff 1.0 2.0 --name-only
+    rsp: ""
+  - cmd: git ls-tree -r 2.0
+    rsp: |
+      100644 blob b7b39845b55fb4d45d58ba86ef4527917877d556    services/bl01t-ea-test-01/Chart.yaml
+  - cmd: git clone https://github.com/epics-containers/bl01t-services -b 1.0 /tmp/ec_tests
+    rsp: ""
+  - cmd: argocd app get namespace/bl01t
+    rsp: |
+      spec:
+        source:
+          repoURL: https://github.com/test/example-deployment.git
+          path: apps
+  - cmd: argocd app list --app-namespace namespace -o yaml
+    rsp: |
+      - metadata:
+          creationTimestamp: "2024-07-12T13:42:50Z"
+          name: bl01t-ea-test-01
+        spec:
+          source:
+            targetRevision: main
+        status:
+          resources:
+            - kind: StatefulSet
+              name: bl01t-ea-test-01
+  - cmd: argocd app manifests namespace/bl01t-ea-test-01 --source live
+    rsp: |
+      ---
+      apiVersion: apps/v1
+      kind: StatefulSet
+      metadata:
+        name: bl01t-ea-test-01
+        creationTimestamp: "2024-07-12T13:52:35Z"
+        labels:
+          test: test_label
+      status:
+        readyReplicas: 1
+  - cmd: argocd app get namespace/bl01t -o yaml
+    rsp: |
+      spec:
+        source:
+          repoURL: https://github.com/test/example-deployment.git
+          path: apps
+  - cmd: git clone --depth=1 https://github.com/test/example-deployment.git /tmp/ec_tests
+    rsp: ""
+  - cmd: git add .
+    rsp: ""
+  - cmd: 'git commit -m "Set services.bl01t-ea-test-01=*'
+    rsp: ""
+  - cmd: git push
+    rsp: ""
+  - cmd: argocd app unset namespace/bl01t -p services.bl01t-ea-test-01
+    rsp: ""
+  - cmd: argocd app get --show-params namespace/bl01t -o json
+    rsp: |
+      {"spec": {"source": {"helm": {"parameters": [
+        {"name": "services.bl01t-ea-test-01.enabled", "value": "false"},
+        {"name": "services.other.enabled", "value": "false"}
+      ]}}}}
+  - cmd: argocd app unset namespace/bl01t -p services.bl01t-ea-test-01.enabled
+    rsp: ""
+  - cmd: argocd app get namespace/bl01t --refresh
+    rsp: ""

tests/test_argocd.py

@@ -19,6 +19,17 @@ def test_deploy(mock_run, ARGOCD, data: Path):
     mock_run.run_cli("deploy bl01t-ea-test-01")
 
 
+def test_deploy_clears_enabled_override(mock_run, ARGOCD, data: Path):
+    # Regression: a lingering `services.X.enabled=false` parameter override
+    # (set by `ec stop --no-commit` or Monitor) must be unset by the next
+    # deploy, otherwise the redeployed service stays stopped.
+    mock_run.set_seq(ARGOCD.deploy_clears_override)
+    TMPDIR.mkdir()
+    shutil.copytree(data / "bl01t-services/services", TMPDIR / "services")
+    shutil.copytree(data / "bl01t-deployment/apps", TMPDIR / "apps")
+    mock_run.run_cli("deploy bl01t-ea-test-01")
+
+
 def test_logs(mock_run, ARGOCD):
     mock_run.set_seq(ARGOCD.checks + ARGOCD.logs)
     mock_run.run_cli("logs bl01t-ea-test-01")
@@ -43,19 +54,19 @@ def test_start_commit(mock_run, ARGOCD, data: Path):
 
 def test_start(mock_run, ARGOCD):
     mock_run.set_seq(ARGOCD.checks + ARGOCD.start)
-    mock_run.run_cli("start bl01t-ea-test-01 --no-commit")
+    mock_run.run_cli("start bl01t-ea-test-01")
 
 
 def test_stop_commit(mock_run, ARGOCD, data: Path):
     mock_run.set_seq(ARGOCD.checks + ARGOCD.stop_commit)
     TMPDIR.mkdir()
     shutil.copytree(data / "bl01t-deployment/apps", TMPDIR / "apps")
-    mock_run.run_cli("stop bl01t-ea-test-01")
+    mock_run.run_cli("stop bl01t-ea-test-01 --commit")
 
 
 def test_stop(mock_run, ARGOCD):
     mock_run.set_seq(ARGOCD.checks + ARGOCD.stop)
-    mock_run.run_cli("stop bl01t-ea-test-01 --no-commit")
+    mock_run.run_cli("stop bl01t-ea-test-01")
 
 
 def test_ps(mock_run, ARGOCD):