From 53395ef55734935bb3dda1ba78141ad6affebb9e Mon Sep 17 00:00:00 2001 From: Giles Knap Date: Thu, 14 May 2026 07:30:56 +0000 Subject: [PATCH 1/2] fix(argo): also unset child parameter overrides on push_value push_value only unset the exact key it just wrote, so a parameter override on a child key survived. Concrete failure: Monitor (or `ec stop --no-commit`) sets `services.X.enabled=false`, then `ec deploy X v2` writes `services.X` to values.yaml but the `services.X.enabled=false` override remains and the redeployed service stays stopped. Lift the child-walk pattern from push_remove_key into a shared helper and call it from both functions. --- src/edge_containers_cli/cmds/argo_commands.py | 23 ++--- tests/data/argocd.yaml | 88 +++++++++++++++++++ tests/test_argocd.py | 11 +++ 3 files changed, 112 insertions(+), 10 deletions(-) diff --git a/src/edge_containers_cli/cmds/argo_commands.py b/src/edge_containers_cli/cmds/argo_commands.py index 60a5b1ff..25e812d7 100644 --- a/src/edge_containers_cli/cmds/argo_commands.py +++ b/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 diff --git a/tests/data/argocd.yaml b/tests/data/argocd.yaml index 84b7a041..709cc3b9 100644 --- a/tests/data/argocd.yaml +++ b/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: "" diff --git a/tests/test_argocd.py b/tests/test_argocd.py index 9eff011d..6b635ed3 100644 --- a/tests/test_argocd.py +++ b/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") From fcd6215c8070e4222f4f7c061dc06daa927c1e05 Mon Sep 17 00:00:00 2001 From: Giles Knap Date: Thu, 14 May 2026 07:32:03 +0000 Subject: [PATCH 2/2] feat(start/stop): default to transient parameter override Flip --commit from on-by-default to opt-in. The default behaviour now matches `ec stop --no-commit` (transient `argocd app set` on the parent app's helm.parameters), which is the same mechanism argocd-monitor's Stop/Start button will use. Pass --commit when you want the change written to the deployment repo for an audit trail. Also aligns ArgoCommands.start/stop kwarg defaults with the abstract base, k8s, and demo backends, which were already commit=False. --- src/edge_containers_cli/cli.py | 8 ++++++-- src/edge_containers_cli/cmds/argo_commands.py | 4 ++-- tests/test_argocd.py | 6 +++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/edge_containers_cli/cli.py b/src/edge_containers_cli/cli.py index 5c9d8bc8..147908b6 100644 --- a/src/edge_containers_cli/cli.py +++ b/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: diff --git a/src/edge_containers_cli/cmds/argo_commands.py b/src/edge_containers_cli/cmds/argo_commands.py index 25e812d7..4c5a4487 100644 --- a/src/edge_containers_cli/cmds/argo_commands.py +++ b/src/edge_containers_cli/cmds/argo_commands.py @@ -236,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) diff --git a/tests/test_argocd.py b/tests/test_argocd.py index 6b635ed3..c133dbfe 100644 --- a/tests/test_argocd.py +++ b/tests/test_argocd.py @@ -54,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):