Skip to content

Commit 59c506a

Browse files
authored
refactor: Replace get_user_input with ask_for_user_consent and merge --quiet with --force. (#811)
* feat: Add --quiet flag * Bump default Kueue version to 0.14.2 * feat: Prepare for Kueue upgrade * Add user_input_test.py * testing: CommandsTester updates * refactor: Simplify kueue_manager_test with CommandsTester * Add user_input docs * Add unit tests * Kueue v0.14.3 * Apply review feedback * Fix import * refactor: Replace `get_user_input` with `ask_for_user_consent` and merge --quiet with --force. * nits * not node_pools_to_update_WI * Apply SikaGrr suggestions
1 parent 41b37e5 commit 59c506a

File tree

9 files changed

+159
-56
lines changed

9 files changed

+159
-56
lines changed

src/xpk/commands/cluster.py

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@
7171
)
7272
from ..core.vertex import create_vertex_tensorboard
7373
from ..core.workload import get_workload_list
74-
from ..utils.console import get_user_input, xpk_exit, xpk_print
74+
from ..utils.console import ask_for_user_consent, xpk_exit, xpk_print
7575
from ..utils.file import write_tmp_file
76-
from ..utils.execution_context import is_dry_run
76+
from ..utils.execution_context import is_dry_run, is_quiet
7777
from ..utils.validation import validate_dependencies_list, SystemDependency, should_validate_dependencies
7878
from . import cluster_gcluster
7979
from .common import set_cluster_command, validate_sub_slicing_system
@@ -1056,7 +1056,7 @@ def run_gke_cluster_delete_command(args) -> int:
10561056
Returns:
10571057
0 if successful and 1 otherwise.
10581058
"""
1059-
if not args.force:
1059+
if not is_quiet():
10601060
xpk_print('Get the name of the workloads in the cluster.')
10611061
args.filter_by_status = 'EVERYTHING'
10621062
return_code, return_value = get_workload_list(args)
@@ -1067,10 +1067,9 @@ def run_gke_cluster_delete_command(args) -> int:
10671067
# Ignore Column Names line.
10681068
if len(return_value) > 1:
10691069
workloads = [x.split(' ')[0] for x in return_value.splitlines()][1:]
1070-
if workloads and not get_user_input(
1070+
if workloads and not ask_for_user_consent(
10711071
f'Planning to delete {len(workloads)} workloads in the cluster'
1072-
f' {args.cluster} including {workloads}. \nDo you wish to delete: y'
1073-
' (yes) / n (no):\n'
1072+
f' {args.cluster} including {workloads}. \nDo you wish to delete?'
10741073
):
10751074
xpk_print('Skipping delete command.')
10761075
return 0

src/xpk/commands/storage.py

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
list_storages,
5757
print_storages_for_cluster,
5858
)
59-
from ..utils.console import get_user_input, xpk_exit, xpk_print
59+
from ..utils.console import ask_for_user_consent, xpk_exit, xpk_print
6060
from ..utils.kubectl import apply_kubectl_manifest
6161
from ..utils.execution_context import is_dry_run
6262
from ..utils.validation import validate_dependencies_list, SystemDependency, should_validate_dependencies
@@ -133,15 +133,13 @@ def storage_delete(args: Namespace) -> None:
133133
if storage.bucket.startswith(filestore_instance_name)
134134
]
135135

136-
if children and not args.force:
137-
detach = get_user_input(
138-
"Deleting a filestore storage will destroy your filestore instance and"
139-
" all its data in all volumes will be lost. Do you wish to delete the"
140-
f" filestore instance {filestore_instance_name}?\n y (yes) / n (no):\n'"
141-
)
142-
if not detach:
143-
xpk_print("Deleting storage canceled.")
144-
xpk_exit(0)
136+
if children and not ask_for_user_consent(
137+
"Deleting a filestore storage will destroy your filestore instance and"
138+
" all its data in all volumes will be lost. Do you wish to delete the"
139+
f" filestore instance {filestore_instance_name}?"
140+
):
141+
xpk_print("Deleting storage canceled.")
142+
xpk_exit(0)
145143

146144
for child in children:
147145
delete_storage_resources(k8s_api_client, child)

src/xpk/commands/workload.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@
9494
tcpx_decorator,
9595
tcpxo_decorator,
9696
)
97-
from ..utils.console import get_user_input, xpk_exit, xpk_print
97+
from ..utils.console import ask_for_user_consent, xpk_exit, xpk_print
9898
from packaging.version import Version
9999
from ..utils.file import write_tmp_file
100100
from ..utils.execution_context import is_dry_run
@@ -781,11 +781,10 @@ def workload_delete(args) -> None:
781781
xpk_exit(return_code)
782782
# Skip the header
783783
workloads = [x.split(' ')[0] for x in return_value.splitlines()][1:]
784-
if workloads and not args.force:
785-
will_delete = get_user_input(
784+
if workloads:
785+
will_delete = ask_for_user_consent(
786786
f'Planning to delete {len(workloads)} workloads in the cluster'
787-
f' {args.cluster} including {workloads}. \nDo you wish to delete: y'
788-
' (yes) / n (no):\n'
787+
f' {args.cluster} including {workloads}. \nDo you wish to delete?'
789788
)
790789
else:
791790
workloads = [args.workload]

src/xpk/core/kueue_manager.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import json
2222
from jinja2 import Environment, FileSystemLoader
2323

24-
from ..utils.user_input import ask_for_user_consent
2524
from ..utils.execution_context import is_dry_run
2625
from ..utils.kueue import is_queued_cluster
2726
from kubernetes.utils import parse_quantity
@@ -41,7 +40,7 @@
4140
run_command_with_updates_retry,
4241
)
4342
from ..utils.file import write_tmp_file
44-
from ..utils.console import xpk_print, xpk_exit
43+
from ..utils.console import xpk_print, xpk_exit, ask_for_user_consent
4544
from ..utils.templates import TEMPLATE_PATH, get_templates_absolute_path
4645
from packaging.version import Version
4746

src/xpk/core/nodepool.py

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
"""
1616

1717
from typing import List
18-
from ..utils.console import get_user_input, xpk_print
18+
from ..utils.console import ask_for_user_consent, xpk_print
1919
from ..utils.topology import get_topology_product, is_topology_valid
2020
from .capacity import (
2121
AUTOPROVISIONING_CONFIG_VALUE,
@@ -110,6 +110,7 @@ def run_gke_node_pool_create_command(
110110
existing_node_pool_names, args.cluster, desired_node_pool_count
111111
)
112112

113+
node_pools_to_delete = []
113114
node_pools_to_remain = []
114115
delete_commands = []
115116
delete_task_names = []
@@ -186,14 +187,10 @@ def run_gke_node_pool_create_command(
186187
# when cluster is getting updated from 'x' device_type/gke_accelerator to 'y' device_type/gke_accelerator.
187188
# In that case, '{args.cluster}-np-i' nodepool will be re-created for 'y' device_type/gke_accelerator.
188189
if delete_commands:
189-
will_delete = True
190-
if node_pools_to_delete and not args.force:
191-
will_delete = get_user_input(
192-
f'Planning to delete {len(node_pools_to_delete)} node pools including'
193-
f' {node_pools_to_delete}. \nDo you wish to delete: y (yes) / n'
194-
' (no):\n'
195-
)
196-
if not will_delete:
190+
if node_pools_to_delete and not ask_for_user_consent(
191+
f'Planning to delete {len(node_pools_to_delete)} node pools including'
192+
f' {node_pools_to_delete}. \nDo you wish to delete?'
193+
):
197194
xpk_print(
198195
'You have requested to not delete the existing nodepools in the'
199196
' cluster. There will be no change to the cluster.'
@@ -215,18 +212,15 @@ def run_gke_node_pool_create_command(
215212

216213
# Enable Workload Identity on existing Nodepools
217214
if update_WI_commands:
218-
will_update_WI = True
219-
if node_pools_to_update_WI and not args.force:
220-
will_update_WI = get_user_input(
221-
'Planning to enable Workload Identity Federation on'
222-
f' {len(node_pools_to_update_WI)} existing node pools including'
223-
f' {node_pools_to_update_WI}.This immediately enables Workload'
224-
' Identity Federation for GKE for any workloads running in the node'
225-
' pool. Also, xpk does not support disabling Workload Identity on'
226-
' clusters that have it enabled already \nDo you wish to update: y'
227-
' (yes) / n (no):\n'
228-
)
229-
if not will_update_WI:
215+
will_update_WI = not node_pools_to_update_WI or ask_for_user_consent(
216+
'Planning to enable Workload Identity Federation on'
217+
f' {len(node_pools_to_update_WI)} existing node pools including'
218+
f' {node_pools_to_update_WI}. This immediately enables Workload'
219+
' Identity Federation for GKE for any workloads running in the node'
220+
' pool. Also, xpk does not support disabling Workload Identity on'
221+
' clusters that have it enabled already \nDo you wish to update?'
222+
)
223+
if will_update_WI:
230224
for i, command in enumerate(update_WI_commands):
231225
xpk_print(
232226
f'To complete {update_WI_task_names[i]} we are executing {command}'

src/xpk/core/nodepool_test.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ def mock_nodepool_dependencies(mocker):
145145
"xpk.core.nodepool.get_cluster_location", return_value="us-central1"
146146
)
147147
mocker.patch("xpk.core.nodepool.run_commands", return_value=0)
148-
mocker.patch("xpk.core.nodepool.get_user_input", return_value=True)
148+
mocker.patch("xpk.core.nodepool.ask_for_user_consent", return_value=True)
149149
mock_is_topology_valid = mocker.patch("xpk.core.nodepool.is_topology_valid")
150150
mock_ensure_resource_policy = mocker.patch(
151151
"xpk.core.nodepool.ensure_resource_policy_exists"

src/xpk/main.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,10 @@ def main() -> None:
7070
main_args.enable_ray_cluster = False
7171
set_context(
7272
dry_run_value='dry_run' in main_args and main_args.dry_run,
73-
quiet_value='quiet' in main_args and main_args.quiet,
73+
quiet_value=(
74+
('quiet' in main_args and main_args.quiet)
75+
or ('force' in main_args and main_args.force)
76+
),
7477
)
7578
generate_client_id()
7679
print_xpk_hello()

src/xpk/utils/console.py

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616

1717
import sys
1818
from typing import NoReturn
19+
from typing import Literal
20+
21+
from .execution_context import is_quiet
1922

2023

2124
def xpk_print(*args, **kwargs):
@@ -25,7 +28,7 @@ def xpk_print(*args, **kwargs):
2528
*args: user provided print args.
2629
**kwargs: user provided print args.
2730
"""
28-
sys.stdout.write('[XPK] ')
31+
sys.stdout.write("[XPK] ")
2932
print(*args, **kwargs)
3033
sys.stdout.flush()
3134

@@ -37,20 +40,36 @@ def xpk_exit(error_code) -> NoReturn:
3740
error_code: If the code provided is zero, then no issues occurred.
3841
"""
3942
if error_code == 0:
40-
xpk_print('Exiting XPK cleanly')
43+
xpk_print("Exiting XPK cleanly")
4144
sys.exit(0)
4245
else:
43-
xpk_print(f'XPK failed, error code {error_code}')
46+
xpk_print(f"XPK failed, error code {error_code}")
4447
sys.exit(error_code)
4548

4649

47-
def get_user_input(input_msg):
48-
"""Function to get the user input for a prompt.
50+
def ask_for_user_consent(
51+
question: str, default_option: Literal["Y", "N"] = "N"
52+
) -> bool:
53+
"""Prompts user with the given question, asking for a yes/no answer and returns a relevant boolean.
54+
Important: immediatelly returns `True` in quiet mode!
55+
56+
Example prompt for `question='Continue?'`: `[XPK] Continue? (y/N): `.
4957
5058
Args:
51-
input_msg: message to be displayed by the prompt.
52-
Returns:
53-
True if user enter y or yes at the prompt, False otherwise.
59+
question: The question to ask the user.
60+
default_option: Option to use when user response is empty.
5461
"""
55-
user_input = input(input_msg)
56-
return user_input in ('y', 'yes')
62+
if is_quiet():
63+
return True
64+
65+
options = "y/N" if default_option == "N" else "Y/n"
66+
prompt = f"[XPK] {question} ({options}): "
67+
68+
while True:
69+
user_input = input(prompt) or default_option
70+
if user_input.lower() in ["yes", "y"]:
71+
return True
72+
elif user_input.lower() in ["no", "n"]:
73+
return False
74+
else:
75+
xpk_print("Invalid input. Please enter: yes/no/y/n.")

src/xpk/utils/console_test.py

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
"""
2+
Copyright 2025 Google LLC
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
https://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
"""
16+
17+
from unittest.mock import MagicMock, patch
18+
import pytest
19+
from pytest_mock import MockerFixture
20+
21+
from xpk.utils.console import ask_for_user_consent
22+
23+
24+
@pytest.fixture(autouse=True)
25+
def mock_is_quiet(mocker: MockerFixture):
26+
return mocker.patch("xpk.utils.console.is_quiet", return_value=False)
27+
28+
29+
@pytest.mark.parametrize(
30+
"user_input,expected",
31+
[
32+
("yes", True),
33+
("y", True),
34+
("Y", True),
35+
("Yes", True),
36+
("YES", True),
37+
("no", False),
38+
("n", False),
39+
("N", False),
40+
("No", False),
41+
("NO", False),
42+
],
43+
)
44+
@patch("xpk.utils.console.input")
45+
def test_ask_for_user_consent(mock_input: MagicMock, user_input, expected):
46+
mock_input.return_value = user_input
47+
48+
assert ask_for_user_consent("Test question?") is expected
49+
50+
51+
def fake_input_factory(user_inputs: list[str]):
52+
def fake_input(prompt: str) -> str:
53+
return user_inputs.pop(0)
54+
55+
return fake_input
56+
57+
58+
@patch("xpk.utils.console.input", wraps=fake_input_factory(["invalid", "y"]))
59+
def test_ask_for_user_consent_invalid_input(mock_input: MagicMock):
60+
agreed = ask_for_user_consent("Test question?")
61+
62+
assert agreed is True
63+
assert mock_input.call_count == 2
64+
65+
66+
@patch("xpk.utils.console.input", return_value="")
67+
def test_ask_for_user_consent_default_No(mock_input: MagicMock):
68+
agreed = ask_for_user_consent("Test question?", default_option="N")
69+
70+
assert agreed is False
71+
mock_input.assert_called_once_with("[XPK] Test question? (y/N): ")
72+
73+
74+
@patch("xpk.utils.console.input", return_value="")
75+
def test_ask_for_user_consent_default_Yes(mock_input: MagicMock):
76+
agreed = ask_for_user_consent("Test question?", default_option="Y")
77+
78+
assert agreed is True
79+
mock_input.assert_called_once_with("[XPK] Test question? (Y/n): ")
80+
81+
82+
@patch("xpk.utils.console.input")
83+
def test_ask_for_user_consent_with_quiet_mode_always_agrees(
84+
mock_input: MagicMock,
85+
mock_is_quiet: MagicMock,
86+
):
87+
mock_is_quiet.return_value = True
88+
89+
agreed = ask_for_user_consent("Test question?", default_option="N")
90+
91+
assert agreed is True
92+
mock_input.assert_not_called()

0 commit comments

Comments
 (0)