Skip to content

Commit 91ba1da

Browse files
fix antipatterns
1 parent 10b78a2 commit 91ba1da

File tree

6 files changed

+182
-74
lines changed

6 files changed

+182
-74
lines changed

keras_remote/cli/commands/down.py

Lines changed: 5 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,18 @@
11
"""keras-remote down command — tear down infrastructure."""
22

33
import click
4-
import pulumi.automation as auto
54

65
from keras_remote.cli.config import InfraConfig
76
from keras_remote.cli.constants import DEFAULT_CLUSTER_NAME, DEFAULT_ZONE
8-
from keras_remote.cli.infra.program import create_program
9-
from keras_remote.cli.infra.stack_manager import get_stack
10-
from keras_remote.cli.output import banner, console, success, warning
7+
from keras_remote.cli.infra.state import apply_destroy
8+
from keras_remote.cli.options import common_options
9+
from keras_remote.cli.output import banner, console, warning
1110
from keras_remote.cli.prerequisites_check import check_all
1211
from keras_remote.cli.prompts import resolve_project
1312

1413

1514
@click.command()
16-
@click.option(
17-
"--project",
18-
envvar="KERAS_REMOTE_PROJECT",
19-
default=None,
20-
help="GCP project ID [env: KERAS_REMOTE_PROJECT]",
21-
)
22-
@click.option(
23-
"--zone",
24-
envvar="KERAS_REMOTE_ZONE",
25-
default=None,
26-
help=(f"GCP zone [env: KERAS_REMOTE_ZONE, default: {DEFAULT_ZONE}]"),
27-
)
28-
@click.option(
29-
"--cluster",
30-
"cluster_name",
31-
envvar="KERAS_REMOTE_CLUSTER",
32-
default=None,
33-
help="GKE cluster name [default: keras-remote-cluster]",
34-
)
15+
@common_options
3516
@click.option("--yes", "-y", is_flag=True, help="Skip confirmation prompt")
3617
def down(project, zone, cluster_name, yes):
3718
"""Tear down keras-remote GCP infrastructure."""
@@ -60,19 +41,7 @@ def down(project, zone, cluster_name, yes):
6041
console.print()
6142

6243
config = InfraConfig(project=project, zone=zone, cluster_name=cluster_name)
63-
64-
# Pulumi destroy
65-
try:
66-
# Minimal config to load the stack — accelerator is not
67-
# needed for destroy since the stack already has its state.
68-
program = create_program(config)
69-
stack = get_stack(program, config)
70-
console.print("[bold]Destroying Pulumi-managed resources...[/bold]\n")
71-
result = stack.destroy(on_output=print)
72-
console.print()
73-
success(f"Pulumi destroy complete. {result.summary.resource_changes}")
74-
except auto.errors.CommandError as e:
75-
warning(f"Pulumi destroy encountered an issue: {e}")
44+
apply_destroy(config)
7645

7746
# Summary
7847
console.print()
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
"""Tests for keras_remote.cli.commands.down — destroy infrastructure."""
2+
3+
from unittest import mock
4+
5+
from absl.testing import absltest
6+
from click.testing import CliRunner
7+
8+
from keras_remote.cli.commands.down import down
9+
10+
# Shared CLI args that skip interactive prompts.
11+
_CLI_ARGS = [
12+
"--project",
13+
"test-project",
14+
"--zone",
15+
"us-central2-b",
16+
"--yes",
17+
]
18+
19+
# Patches applied to every test to bypass prerequisites and infrastructure.
20+
_BASE_PATCHES = {
21+
"check_all": mock.patch("keras_remote.cli.commands.down.check_all"),
22+
"resolve_project": mock.patch(
23+
"keras_remote.cli.commands.down.resolve_project",
24+
return_value="test-project",
25+
),
26+
"apply_destroy": mock.patch(
27+
"keras_remote.cli.commands.down.apply_destroy", return_value=True
28+
),
29+
}
30+
31+
32+
def _start_patches(test_case):
33+
"""Start all base patches and return a dict of mock objects."""
34+
mocks = {}
35+
for name, patcher in _BASE_PATCHES.items():
36+
mocks[name] = test_case.enterContext(patcher)
37+
return mocks
38+
39+
40+
class DownCommandTest(absltest.TestCase):
41+
def setUp(self):
42+
super().setUp()
43+
self.runner = CliRunner()
44+
self.mocks = _start_patches(self)
45+
46+
def test_successful_destroy(self):
47+
"""Successful destroy — exit code 0, 'Cleanup Complete' shown."""
48+
result = self.runner.invoke(down, _CLI_ARGS)
49+
50+
self.assertEqual(result.exit_code, 0, result.output)
51+
self.assertIn("Cleanup Complete", result.output)
52+
self.mocks["apply_destroy"].assert_called_once()
53+
54+
def test_destroy_failure_still_shows_summary(self):
55+
"""apply_destroy returns False — summary still displayed."""
56+
self.mocks["apply_destroy"].return_value = False
57+
58+
result = self.runner.invoke(down, _CLI_ARGS)
59+
60+
self.assertEqual(result.exit_code, 0, result.output)
61+
self.assertIn("Cleanup Complete", result.output)
62+
self.assertIn("Check manually", result.output)
63+
64+
def test_abort_on_no_confirmation(self):
65+
"""User declines confirmation — apply_destroy not called."""
66+
args = ["--project", "test-project", "--zone", "us-central2-b"]
67+
68+
self.runner.invoke(down, args, input="n\n")
69+
70+
self.mocks["apply_destroy"].assert_not_called()
71+
72+
def test_yes_flag_skips_confirmation(self):
73+
"""--yes flag skips confirmation prompt."""
74+
result = self.runner.invoke(down, _CLI_ARGS)
75+
76+
self.assertEqual(result.exit_code, 0, result.output)
77+
self.mocks["apply_destroy"].assert_called_once()
78+
79+
def test_config_passed_correctly(self):
80+
"""InfraConfig args match CLI options."""
81+
args = [
82+
"--project",
83+
"my-proj",
84+
"--zone",
85+
"europe-west1-b",
86+
"--cluster",
87+
"my-cluster",
88+
"--yes",
89+
]
90+
91+
result = self.runner.invoke(down, args)
92+
93+
self.assertEqual(result.exit_code, 0, result.output)
94+
config = self.mocks["apply_destroy"].call_args[0][0]
95+
self.assertEqual(config.project, "my-proj")
96+
self.assertEqual(config.zone, "europe-west1-b")
97+
self.assertEqual(config.cluster_name, "my-cluster")
98+
99+
def test_resolve_project_allow_create_false(self):
100+
"""When --project not given, resolve_project(allow_create=False) is called."""
101+
args = ["--zone", "us-central1-a", "--yes"]
102+
103+
result = self.runner.invoke(down, args)
104+
105+
self.assertEqual(result.exit_code, 0, result.output)
106+
self.mocks["resolve_project"].assert_called_once_with(allow_create=False)
107+
108+
109+
if __name__ == "__main__":
110+
absltest.main()

keras_remote/cli/commands/status.py

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import click
44

55
from keras_remote.cli.infra.state import load_state
6+
from keras_remote.cli.options import common_options
67
from keras_remote.cli.output import (
78
banner,
89
console,
@@ -12,25 +13,7 @@
1213

1314

1415
@click.command()
15-
@click.option(
16-
"--project",
17-
envvar="KERAS_REMOTE_PROJECT",
18-
default=None,
19-
help="GCP project ID [env: KERAS_REMOTE_PROJECT]",
20-
)
21-
@click.option(
22-
"--zone",
23-
envvar="KERAS_REMOTE_ZONE",
24-
default=None,
25-
help="GCP zone [env: KERAS_REMOTE_ZONE]",
26-
)
27-
@click.option(
28-
"--cluster",
29-
"cluster_name",
30-
envvar="KERAS_REMOTE_CLUSTER",
31-
default=None,
32-
help="GKE cluster name [default: keras-remote-cluster]",
33-
)
16+
@common_options
3417
def status(project, zone, cluster_name):
3518
"""Show current keras-remote infrastructure state."""
3619
banner("keras-remote Status")

keras_remote/cli/commands/up.py

Lines changed: 2 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
install_lws,
1313
)
1414
from keras_remote.cli.infra.state import apply_update, load_state
15+
from keras_remote.cli.options import common_options
1516
from keras_remote.cli.output import (
1617
banner,
1718
config_summary,
@@ -26,31 +27,13 @@
2627

2728

2829
@click.command()
29-
@click.option(
30-
"--project",
31-
envvar="KERAS_REMOTE_PROJECT",
32-
default=None,
33-
help="GCP project ID [env: KERAS_REMOTE_PROJECT]",
34-
)
35-
@click.option(
36-
"--zone",
37-
envvar="KERAS_REMOTE_ZONE",
38-
default=None,
39-
help=(f"GCP zone [env: KERAS_REMOTE_ZONE, default: {DEFAULT_ZONE}]"),
40-
)
30+
@common_options
4131
@click.option(
4232
"--accelerator",
4333
default=None,
4434
help="Accelerator spec: cpu, t4, l4, a100, a100-80gb, h100, "
4535
"v5litepod, v5p, v6e, v3",
4636
)
47-
@click.option(
48-
"--cluster",
49-
"cluster_name",
50-
envvar="KERAS_REMOTE_CLUSTER",
51-
default=None,
52-
help="GKE cluster name [default: keras-remote-cluster]",
53-
)
5437
@click.option("--yes", "-y", is_flag=True, help="Skip confirmation prompt")
5538
def up(project, zone, accelerator, cluster_name, yes):
5639
"""Provision GCP infrastructure for keras-remote."""

keras_remote/cli/infra/state.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,3 +119,27 @@ def apply_update(config):
119119
console.print()
120120
warning(f"Pulumi update encountered an issue: {e}")
121121
return False
122+
123+
124+
def apply_destroy(config):
125+
"""Destroy all Pulumi-managed resources for the given config.
126+
127+
Args:
128+
config: A fully populated InfraConfig.
129+
130+
Returns:
131+
True if the destroy succeeded, False otherwise.
132+
"""
133+
program = create_program(config)
134+
stack = get_stack(program, config)
135+
136+
console.print("\n[bold]Destroying Pulumi-managed resources...[/bold]\n")
137+
try:
138+
result = stack.destroy(on_output=print)
139+
console.print()
140+
success(f"Pulumi destroy complete. {result.summary.resource_changes}")
141+
return True
142+
except auto.errors.CommandError as e:
143+
console.print()
144+
warning(f"Pulumi destroy encountered an issue: {e}")
145+
return False

keras_remote/cli/infra/state_test.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,5 +131,44 @@ def test_passes_config_to_create_program(self):
131131
self.mock_create.assert_called_once_with(config)
132132

133133

134+
class ApplyDestroyTest(absltest.TestCase):
135+
def setUp(self):
136+
super().setUp()
137+
self.mock_create = self.enterContext(
138+
mock.patch.object(state, "create_program")
139+
)
140+
self.mock_get_stack = self.enterContext(
141+
mock.patch.object(state, "get_stack")
142+
)
143+
self.mock_stack = mock.MagicMock()
144+
self.mock_stack.destroy.return_value.summary.resource_changes = {
145+
"delete": 1
146+
}
147+
self.mock_get_stack.return_value = self.mock_stack
148+
149+
def test_success_returns_true(self):
150+
config = mock.MagicMock()
151+
152+
result = state.apply_destroy(config)
153+
154+
self.assertTrue(result)
155+
self.mock_stack.destroy.assert_called_once()
156+
157+
def test_failure_returns_false(self):
158+
self.mock_stack.destroy.side_effect = pulumi_errors.CommandError("failed")
159+
config = mock.MagicMock()
160+
161+
result = state.apply_destroy(config)
162+
163+
self.assertFalse(result)
164+
165+
def test_passes_config_to_create_program(self):
166+
config = mock.MagicMock()
167+
168+
state.apply_destroy(config)
169+
170+
self.mock_create.assert_called_once_with(config)
171+
172+
134173
if __name__ == "__main__":
135174
absltest.main()

0 commit comments

Comments
 (0)