From 7a412d713f87d844202ec00fb4615143a81e93f9 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 7 Mar 2024 12:55:33 +0100 Subject: [PATCH 1/5] Greet before asking questions It's just common courtesy. --- src/appsignal/cli/install.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/appsignal/cli/install.py b/src/appsignal/cli/install.py index 26d9a03..861535e 100644 --- a/src/appsignal/cli/install.py +++ b/src/appsignal/cli/install.py @@ -36,16 +36,15 @@ def init_parser(parser: ArgumentParser) -> None: def run(self) -> int: options = Options() - # Make sure to show input prompts before the welcome text. - options["name"] = self._name - options["push_api_key"] = self._push_api_key - print("👋 Welcome to the AppSignal for Python installer!") print() print("Reach us at support@appsignal.com for support") print("Documentation available at https://docs.appsignal.com/python") print() + options["name"] = self._name + options["push_api_key"] = self._push_api_key + print() print("Validating API key") From fdbc0b8abb94722ddf27bd4c30a572208d0d11ff Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 7 Mar 2024 14:56:31 +0100 Subject: [PATCH 2/5] Do not use `@cached_property` for prompts Using `@cached_property` to define these prompting functions (which are, fundamentally, not really properties at all) makes it a lot harder to reason about what happens when you invoke them. Furthermore, as far as I'm aware, there's no documented way to clear the cache for a cached property. This change is, in the wise words of Mickey Mouse, "a surprise tool that will help us later". --- src/appsignal/cli/command.py | 12 +++++------- src/appsignal/cli/demo.py | 13 ++++++------- src/appsignal/cli/install.py | 18 +++++++++--------- 3 files changed, 20 insertions(+), 23 deletions(-) diff --git a/src/appsignal/cli/command.py b/src/appsignal/cli/command.py index 294f06c..ef4b8b2 100644 --- a/src/appsignal/cli/command.py +++ b/src/appsignal/cli/command.py @@ -5,6 +5,7 @@ from argparse import ArgumentParser, Namespace from dataclasses import dataclass from functools import cached_property +from typing import NoReturn from ..client import Client from .config import _client_from_config_file @@ -48,22 +49,19 @@ def _environment_argument(parser: ArgumentParser) -> None: def run(self) -> int: raise NotImplementedError - @cached_property - def _push_api_key(self) -> str | None: + def _push_api_key(self) -> str: key = self.args.push_api_key while not key: key = input("Please enter your Push API key: ") return key - @cached_property - def _name(self) -> str | None: + def _name(self) -> str: name = self.args.application while not name: name = input("Please enter the name of your application: ") return name - @cached_property - def _environment(self) -> str | None: + def _environment(self) -> str: environment = self.args.environment while not environment: environment = input( @@ -71,7 +69,7 @@ def _environment(self) -> str | None: ) return environment - def _client_from_config_file(self) -> Client | None: + def _client_from_config_file(self) -> Client | NoReturn: try: return _client_from_config_file() except Exception as error: diff --git a/src/appsignal/cli/demo.py b/src/appsignal/cli/demo.py index dea4e53..ebaaf27 100644 --- a/src/appsignal/cli/demo.py +++ b/src/appsignal/cli/demo.py @@ -36,15 +36,14 @@ def run(self) -> int: if push_api_key: client._config.options["push_api_key"] = push_api_key else: - # Prompt for all the required config in a sensible order - self._name # noqa: B018 - self._environment # noqa: B018 - self._push_api_key # noqa: B018 + name = self._name() + environment = self._environment() + push_api_key = self._push_api_key() client = Client( active=True, - name=self._name, - environment=self._environment, - push_api_key=self._push_api_key, + name=name, + environment=environment, + push_api_key=push_api_key, ) if not client._config.is_active(): diff --git a/src/appsignal/cli/install.py b/src/appsignal/cli/install.py index 861535e..3544d00 100644 --- a/src/appsignal/cli/install.py +++ b/src/appsignal/cli/install.py @@ -42,8 +42,8 @@ def run(self) -> int: print("Documentation available at https://docs.appsignal.com/python") print() - options["name"] = self._name - options["push_api_key"] = self._push_api_key + options["name"] = self._name() + options["push_api_key"] = self._push_api_key() print() @@ -53,7 +53,7 @@ def run(self) -> int: if validation_result == "valid": print("API key is valid!") elif validation_result == "invalid": - print(f"API key {self._push_api_key} is not valid ") + print(f"API key {options["push_api_key"]} is not valid ") print("please get a new one on https://appsignal.com") return 1 else: @@ -68,14 +68,14 @@ def run(self) -> int: if self._should_write_file(): print(f"Writing the {INSTALL_FILE_NAME} configuration file...") - self._write_file() + self._write_file(options) print() print() client = Client( active=True, - name=self._name, - push_api_key=self._push_api_key, + name=options["name"], + push_api_key=options["push_api_key"], ) client.start() Demo.transmit() @@ -108,11 +108,11 @@ def _input_should_overwrite_file(self) -> bool: print('Please answer "y" (yes) or "n" (no)') return self._input_should_overwrite_file() - def _write_file(self) -> None: + def _write_file(self, options: Options) -> None: with open(INSTALL_FILE_NAME, "w") as f: file_contents = INSTALL_FILE_TEMPLATE.format( - name=self._name, - push_api_key=self._push_api_key, + name=options["name"], + push_api_key=options["push_api_key"], ) f.write(file_contents) From 4b5f6b35d588aba8c13563f34c8b73acc831e189 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 7 Mar 2024 15:07:54 +0100 Subject: [PATCH 3/5] Ask for the push API key again if it's invalid If the push API key does not validate, ask for it again instead of kicking the user out of the installer. Move the logic out of the install command and to its own function. Spice it up with some emojis (they're free!) --- src/appsignal/cli/command.py | 30 ++++++++++++++++++++++++++++-- src/appsignal/cli/install.py | 24 ++---------------------- 2 files changed, 30 insertions(+), 24 deletions(-) diff --git a/src/appsignal/cli/command.py b/src/appsignal/cli/command.py index ef4b8b2..809ee5e 100644 --- a/src/appsignal/cli/command.py +++ b/src/appsignal/cli/command.py @@ -4,10 +4,11 @@ from abc import ABC, abstractmethod from argparse import ArgumentParser, Namespace from dataclasses import dataclass -from functools import cached_property from typing import NoReturn from ..client import Client +from ..config import Config, Options +from ..push_api_key_validator import PushApiKeyValidator from .config import _client_from_config_file from .exit_error import ExitError @@ -55,6 +56,31 @@ def _push_api_key(self) -> str: key = input("Please enter your Push API key: ") return key + def _valid_push_api_key(self) -> str | NoReturn: + while True: + key = self._push_api_key() + config = Config(Options(push_api_key=key)) + print("Validating API key...") + print() + + validation_result = PushApiKeyValidator.validate(config) + if validation_result == "valid": + print("✅ API key is valid!") + return key + + if validation_result == "invalid": + print(f"❌ API key {key} is not valid.") + print("Please get a new one on https://appsignal.com and try again.") + print() + self.args.push_api_key = None + else: + print( + "Error while validating Push API key. HTTP status code: " + "{validation_result}" + ) + print("Reach us at support@appsignal.com for support.") + raise ExitError(1) + def _name(self) -> str: name = self.args.application while not name: @@ -69,7 +95,7 @@ def _environment(self) -> str: ) return environment - def _client_from_config_file(self) -> Client | NoReturn: + def _client_from_config_file(self) -> Client | None | NoReturn: try: return _client_from_config_file() except Exception as error: diff --git a/src/appsignal/cli/install.py b/src/appsignal/cli/install.py index 3544d00..35584c4 100644 --- a/src/appsignal/cli/install.py +++ b/src/appsignal/cli/install.py @@ -4,8 +4,7 @@ from argparse import ArgumentParser from ..client import Client -from ..config import Config, Options -from ..push_api_key_validator import PushApiKeyValidator +from ..config import Options from .command import AppsignalCLICommand from .demo import Demo @@ -43,29 +42,10 @@ def run(self) -> int: print() options["name"] = self._name() - options["push_api_key"] = self._push_api_key() + options["push_api_key"] = self._valid_push_api_key() print() - print("Validating API key") - print() - validation_result = PushApiKeyValidator.validate(Config(options)) - if validation_result == "valid": - print("API key is valid!") - elif validation_result == "invalid": - print(f"API key {options["push_api_key"]} is not valid ") - print("please get a new one on https://appsignal.com") - return 1 - else: - print( - "Error while validating Push API key. HTTP status code: " - "{validation_result}" - ) - print( - "Reach us at support@appsignal.com for support if this keeps happening." - ) - return 1 - if self._should_write_file(): print(f"Writing the {INSTALL_FILE_NAME} configuration file...") self._write_file(options) From eb69dded3bcb6e1c3ac7d80f8dcaf6ee29adb03e Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 7 Mar 2024 16:13:02 +0100 Subject: [PATCH 4/5] Handle unexpected validation errors Handle unexpected validation errors, such as AppSignal's servers not being reachable. --- src/appsignal/cli/command.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/appsignal/cli/command.py b/src/appsignal/cli/command.py index 809ee5e..5716ec1 100644 --- a/src/appsignal/cli/command.py +++ b/src/appsignal/cli/command.py @@ -63,7 +63,13 @@ def _valid_push_api_key(self) -> str | NoReturn: print("Validating API key...") print() - validation_result = PushApiKeyValidator.validate(config) + try: + validation_result = PushApiKeyValidator.validate(config) + except Exception as e: + print(f"Error while validating Push API key: {e}") + print("Reach us at support@appsignal.com for support.") + raise ExitError(1) from e + if validation_result == "valid": print("✅ API key is valid!") return key @@ -76,7 +82,7 @@ def _valid_push_api_key(self) -> str | NoReturn: else: print( "Error while validating Push API key. HTTP status code: " - "{validation_result}" + f"{validation_result}" ) print("Reach us at support@appsignal.com for support.") raise ExitError(1) From 4544c88f551795d8ff03d7d69b75438c5c5b81a9 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Thu, 7 Mar 2024 16:31:40 +0100 Subject: [PATCH 5/5] Test asking again for API key Make it so that the `mock_input` context manager can indicate the expectation of an unanswered prompt by using `None` instead of an answer. When this happens, the expected `StopIteration` that indicates that a prompt was asked for which no answer was given is not raised as an error. Delete several duplicates of `mock_input` and move it to a single `utils.py` file. --- src/appsignal/cli/command.py | 5 ++--- tests/cli/__init__.py | 0 tests/cli/test_demo.py | 10 +--------- tests/cli/test_diagnose.py | 10 +--------- tests/cli/test_install.py | 32 ++++++++++++++++++++------------ tests/cli/utils.py | 19 +++++++++++++++++++ 6 files changed, 43 insertions(+), 33 deletions(-) create mode 100644 tests/cli/__init__.py create mode 100644 tests/cli/utils.py diff --git a/src/appsignal/cli/command.py b/src/appsignal/cli/command.py index 5716ec1..32afdf4 100644 --- a/src/appsignal/cli/command.py +++ b/src/appsignal/cli/command.py @@ -4,7 +4,6 @@ from abc import ABC, abstractmethod from argparse import ArgumentParser, Namespace from dataclasses import dataclass -from typing import NoReturn from ..client import Client from ..config import Config, Options @@ -56,7 +55,7 @@ def _push_api_key(self) -> str: key = input("Please enter your Push API key: ") return key - def _valid_push_api_key(self) -> str | NoReturn: + def _valid_push_api_key(self) -> str: while True: key = self._push_api_key() config = Config(Options(push_api_key=key)) @@ -101,7 +100,7 @@ def _environment(self) -> str: ) return environment - def _client_from_config_file(self) -> Client | None | NoReturn: + def _client_from_config_file(self) -> Client | None: try: return _client_from_config_file() except Exception as error: diff --git a/tests/cli/__init__.py b/tests/cli/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/cli/test_demo.py b/tests/cli/test_demo.py index ba070dd..1f234b0 100644 --- a/tests/cli/test_demo.py +++ b/tests/cli/test_demo.py @@ -2,19 +2,11 @@ import os import shutil -from contextlib import contextmanager from appsignal.cli.base import main from appsignal.cli.install import INSTALL_FILE_TEMPLATE - -@contextmanager -def mock_input(mocker, *pairs: tuple[str, str]): - prompt_calls = [mocker.call(prompt) for (prompt, _) in pairs] - answers = [answer for (_, answer) in pairs] - mock = mocker.patch("builtins.input", side_effect=answers) - yield - assert prompt_calls == mock.mock_calls +from .utils import mock_input def test_demo_with_valid_config(mocker, capfd): diff --git a/tests/cli/test_diagnose.py b/tests/cli/test_diagnose.py index 2ce1061..1224ae1 100644 --- a/tests/cli/test_diagnose.py +++ b/tests/cli/test_diagnose.py @@ -2,19 +2,11 @@ import os import shutil -from contextlib import contextmanager from appsignal.cli.base import main from appsignal.cli.install import INSTALL_FILE_TEMPLATE - -@contextmanager -def mock_input(mocker, *pairs: tuple[str, str]): - prompt_calls = [mocker.call(prompt) for (prompt, _) in pairs] - answers = [answer for (_, answer) in pairs] - mock = mocker.patch("builtins.input", side_effect=answers) - yield - assert prompt_calls == mock.mock_calls +from .utils import mock_input def test_diagnose_with_valid_config(mocker, capfd): diff --git a/tests/cli/test_install.py b/tests/cli/test_install.py index 2958f97..1dafc5e 100644 --- a/tests/cli/test_install.py +++ b/tests/cli/test_install.py @@ -2,12 +2,13 @@ import os import shutil -from contextlib import contextmanager from unittest.mock import MagicMock from appsignal.cli.base import main from appsignal.cli.install import INSTALL_FILE_TEMPLATE +from .utils import mock_input + EXPECTED_FILE_CONTENTS = """from appsignal import Appsignal @@ -22,15 +23,6 @@ """ -@contextmanager -def mock_input(mocker, *pairs: tuple[str, str]): - prompt_calls = [mocker.call(prompt) for (prompt, _) in pairs] - answers = [answer for (_, answer) in pairs] - mock = mocker.patch("builtins.input", side_effect=answers) - yield - assert prompt_calls == mock.mock_calls - - def mock_file_operations(mocker, file_exists: bool = False): mocker.patch("os.path.exists", return_value=file_exists) mocker.patch("appsignal.cli.install.open") @@ -47,6 +39,17 @@ def assert_wrote_file_contents(mocker): ) +def assert_did_not_write_file_contents(mocker): + from appsignal.cli import install + + builtins_open: MagicMock = install.open # type: ignore[attr-defined] + assert mocker.call("__appsignal__.py", "w") not in builtins_open.mock_calls + assert ( + mocker.call().__enter__().write(EXPECTED_FILE_CONTENTS) + not in builtins_open.mock_calls + ) + + def assert_wrote_real_file_contents(test_dir, name, push_api_key): with open(os.path.join(test_dir, "__appsignal__.py")) as f: file_contents = INSTALL_FILE_TEMPLATE.format( @@ -159,11 +162,16 @@ def test_install_command_when_file_exists_no_overwrite(mocker): assert install.open.mock_calls == [] # type: ignore[attr-defined] -def test_install_comand_when_api_key_is_not_valid(mocker): +def test_install_command_when_invalid_api_key_ask_again(mocker): + mock_file_operations(mocker) mock_validate_push_api_key_request(mocker, status_code=401) with mock_input( mocker, ("Please enter the name of your application: ", "My app name"), + ("Please enter your Push API key: ", "My push API key"), + ("Please enter your Push API key: ", None), ): - assert main(["install", "--push-api-key=bad-push-api-key"]) == 1 + main(["install"]) + + assert_did_not_write_file_contents(mocker) diff --git a/tests/cli/utils.py b/tests/cli/utils.py new file mode 100644 index 0000000..7842a12 --- /dev/null +++ b/tests/cli/utils.py @@ -0,0 +1,19 @@ +from __future__ import annotations + +from contextlib import contextmanager + + +@contextmanager +def mock_input(mocker, *pairs: tuple[str, str | None]): + expected_unanswered = pairs[-1][1] is None + + prompt_calls = [mocker.call(prompt) for (prompt, _) in pairs] + answers = [answer for (_, answer) in pairs if answer is not None] + mock = mocker.patch("builtins.input", side_effect=answers) + try: + yield + except StopIteration as e: + if not expected_unanswered: + raise e + finally: + assert prompt_calls == mock.mock_calls