From dfe70211f520aa4604ca6dd2c93e9f35dc3e1a76 Mon Sep 17 00:00:00 2001 From: Sabart Otto Date: Thu, 24 Oct 2024 13:10:38 +0200 Subject: [PATCH] Second bunch of code improvements (#7) * Add missing docstrings for return values * Move the settings to one place * Fix the typo in gitignore * Fix the RET505 lint * Provide service arguments as dictionary * Add a doctype to HTML template * Change page default title * Handle various types of responses when celery is disabled --- .gitignore | 2 +- src/tmt_web/api.py | 59 ++++++++++--------- src/tmt_web/generators/json_generator.py | 6 +- .../generators/templates/_base.html.j2 | 6 +- src/tmt_web/generators/yaml_generator.py | 6 +- src/tmt_web/service.py | 20 +++---- src/tmt_web/settings.py | 5 ++ src/tmt_web/utils/git_handler.py | 10 ++-- 8 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 src/tmt_web/settings.py diff --git a/.gitignore b/.gitignore index 9b1452a..1d6f257 100644 --- a/.gitignore +++ b/.gitignore @@ -6,5 +6,5 @@ venv .vscode dist .ruff_cache -.mypy_chache +.mypy_cache __pycache__ diff --git a/src/tmt_web/api.py b/src/tmt_web/api.py index d895b2d..45f6639 100644 --- a/src/tmt_web/api.py +++ b/src/tmt_web/api.py @@ -5,9 +5,9 @@ from fastapi import FastAPI from fastapi.params import Query from pydantic import BaseModel -from starlette.responses import HTMLResponse +from starlette.responses import HTMLResponse, JSONResponse, PlainTextResponse -from tmt_web import service +from tmt_web import service, settings from tmt_web.generators import html_generator app = FastAPI() @@ -98,36 +98,39 @@ def find_test( if plan_url is None and plan_name is None and test_url is None and test_name is None: return "Missing arguments!" # TODO: forward to docs + + service_args = { + "test_url": test_url, + "test_name": test_name, + "test_ref": test_ref, + "plan_url": plan_url, + "plan_name": plan_name, + "plan_ref": plan_ref, + "out_format": out_format, + "test_path": test_path, + "plan_path": plan_path, + } + # Disable Celery if not needed if os.environ.get("USE_CELERY") == "false": - return service.main( - test_url=test_url, - test_name=test_name, - test_ref=test_ref, - plan_url=plan_url, - plan_name=plan_name, - plan_ref=plan_ref, - out_format=out_format, - test_path=test_path, - plan_path=plan_path) - - r = service.main.delay( - test_url=test_url, - test_name=test_name, - test_ref=test_ref, - plan_url=plan_url, - plan_name=plan_name, - plan_ref=plan_ref, - out_format=out_format, - test_path=test_path, - plan_path=plan_path) + response_by_output = { + "html": HTMLResponse, + "json": JSONResponse, + "yaml": PlainTextResponse, + } + + response = response_by_output.get(out_format, PlainTextResponse) + return response(service.main(**service_args)) + + r = service.main.delay(**service_args) # Special handling of response if the format is html + # TODO: Shouldn't be the "yaml" format also covered with a `PlainTextResponse`? if out_format == "html": - status_callback_url = f'{os.getenv("API_HOSTNAME")}/status/html?task-id={r.task_id}' + status_callback_url = f"{settings.API_HOSTNAME}/status/html?task-id={r.task_id}" return HTMLResponse(content=html_generator.generate_status_callback(r, status_callback_url)) - else: - return _to_task_out(r) + + return _to_task_out(r) @app.get("/status") @@ -149,7 +152,7 @@ def status_html(task_id: Annotated[str | None, ) ]) -> HTMLResponse: r = service.main.app.AsyncResult(task_id) - status_callback_url = f'{os.getenv("API_HOSTNAME")}/status/html?task-id={r.task_id}' + status_callback_url = f"{settings.API_HOSTNAME}/status/html?task-id={r.task_id}" return HTMLResponse(content=html_generator.generate_status_callback(r, status_callback_url)) @@ -158,7 +161,7 @@ def _to_task_out(r: AsyncResult) -> TaskOut: # type: ignore [type-arg] id=r.task_id, status=r.status, result=r.traceback if r.failed() else r.result, - status_callback_url=f'{os.getenv("API_HOSTNAME")}/status?task-id={r.task_id}' + status_callback_url="{settings.API_HOSTNAME}/status?task-id={r.task_id}", ) diff --git a/src/tmt_web/generators/json_generator.py b/src/tmt_web/generators/json_generator.py index 47ef5d2..3cf76e0 100644 --- a/src/tmt_web/generators/json_generator.py +++ b/src/tmt_web/generators/json_generator.py @@ -35,7 +35,7 @@ def generate_test_json(test: tmt.Test, logger: tmt.Logger) -> str: :param test: Test object :param logger: tmt.Logger instance - :return: + :return: JSON data for a given test """ data = _create_json_data(test, logger) logger.print("Generating the JSON file...") @@ -48,7 +48,7 @@ def generate_plan_json(plan: tmt.Plan, logger: tmt.Logger) -> str: :param plan: Plan object :param logger: tmt.Logger instance - :return: + :return: JSON data for a given plan """ data = _create_json_data(plan, logger) logger.print("Generating the JSON file...") @@ -62,7 +62,7 @@ def generate_testplan_json(test: tmt.Test, plan: tmt.Plan, logger: tmt.Logger) - :param test: Test object :param plan: Plan object :param logger: tmt.Logger instance - :return: + :return: JSON data for a given test and plan """ logger.print("Generating the JSON file...") data = { diff --git a/src/tmt_web/generators/templates/_base.html.j2 b/src/tmt_web/generators/templates/_base.html.j2 index 83ec3e5..9a52187 100644 --- a/src/tmt_web/generators/templates/_base.html.j2 +++ b/src/tmt_web/generators/templates/_base.html.j2 @@ -1,7 +1,7 @@ - - + + - {{ title | default('HTML File') }} + {{ title | default('Untitled Document') }} diff --git a/src/tmt_web/generators/yaml_generator.py b/src/tmt_web/generators/yaml_generator.py index 08d2e03..b3aed80 100644 --- a/src/tmt_web/generators/yaml_generator.py +++ b/src/tmt_web/generators/yaml_generator.py @@ -14,7 +14,7 @@ def generate_test_yaml(test: tmt.Test, logger: Logger) -> str: :param test: Test object :param logger: tmt.Logger instance - :return: + :return: YAML data for a given test """ yaml_data = tmt.utils.dict_to_yaml(_create_json_data(test, logger)) print_success(logger) @@ -27,7 +27,7 @@ def generate_plan_yaml(plan: tmt.Plan, logger: Logger) -> str: :param plan: Plan object :param logger: tmt.Logger instance - :return: + :return: YAML data for a given plan. """ yaml_data = tmt.utils.dict_to_yaml(_create_json_data(plan, logger)) print_success(logger) @@ -41,7 +41,7 @@ def generate_testplan_yaml(test: tmt.Test, plan: tmt.Plan, logger: Logger) -> st :param test: Test object :param plan: Plan object :param logger: tmt.Logger instance - :return: + :return: YAML data for a given test and plan """ data = { "test": _create_json_data(test, logger), diff --git a/src/tmt_web/service.py b/src/tmt_web/service.py index 50d399f..d6e7ab3 100644 --- a/src/tmt_web/service.py +++ b/src/tmt_web/service.py @@ -1,18 +1,16 @@ import logging -import os import tmt from celery.app import Celery # type: ignore[attr-defined] from tmt.utils import Path # type: ignore[attr-defined] +from tmt_web import settings from tmt_web.generators import html_generator, json_generator, yaml_generator from tmt_web.utils import git_handler logger = tmt.Logger(logging.getLogger("tmt-logger")) -redis_url = os.getenv("REDIS_URL", "redis://localhost:6379") - -app = Celery(__name__, broker=redis_url, backend=redis_url) +app = Celery(__name__, broker=settings.REDIS_URL, backend=settings.REDIS_URL) def get_tree(url: str, name: str, ref: str | None, tree_path: str) -> tmt.base.Tree: @@ -23,7 +21,7 @@ def get_tree(url: str, name: str, ref: str | None, tree_path: str) -> tmt.base.T :param name: Object name :param url: Object url :param tree_path: Object path - :return: + :return: returns a Tree object """ logger.print(f"Cloning the repository for url: {url}") logger.print("Parsing the url and name...") @@ -52,7 +50,8 @@ def process_test_request(test_url: str, return_object: bool, out_format: str) -> str | tmt.Test | None: """ - This function processes the request for a test and returns the HTML file or the Test object. + This function processes the request for a test and returns the data in specified output format + or the Test object. :param test_url: Test url :param test_name: Test name @@ -60,7 +59,7 @@ def process_test_request(test_url: str, :param test_path: Test path :param return_object: Specify if the function should return the HTML file or the Test object :param out_format: Specifies output format - :return: + :return: the data in specified output format or the Test object """ tree = get_tree(test_url, test_name, test_ref, test_path) @@ -93,7 +92,8 @@ def process_plan_request(plan_url: str, return_object: bool, out_format: str) -> str | None | tmt.Plan: """ - This function processes the request for a plan and returns the HTML file or the Plan object. + This function processes the request for a plan and returns the data in specified output format + or the Plan object. :param plan_url: Plan URL :param plan_name: Plan name @@ -101,7 +101,7 @@ def process_plan_request(plan_url: str, :param plan_path: Plan path :param return_object: Specify if the function should return the HTML file or the Plan object :param out_format: Specifies output format - :return: + :return: the data in specified output format or the Plan object """ tree = get_tree(plan_url, plan_name, plan_ref, plan_path) @@ -147,7 +147,7 @@ def process_testplan_request(test_url, :param plan_ref: Plan repo ref :param plan_path: Plan path :param out_format: Specifies output format - :return: + :return: page data in specified output format """ test = process_test_request(test_url, test_name, test_ref, test_path, False, out_format) if not isinstance(test, tmt.Test): diff --git a/src/tmt_web/settings.py b/src/tmt_web/settings.py new file mode 100644 index 0000000..be5faac --- /dev/null +++ b/src/tmt_web/settings.py @@ -0,0 +1,5 @@ +import os + +API_HOSTNAME = os.getenv("API_HOSTNAME", default="") +REDIS_URL = os.getenv("REDIS_URL", default="redis://localhost:6379") +CLONE_DIR_PATH = os.getenv("CLONE_DIR_PATH", default="./.repos/") diff --git a/src/tmt_web/utils/git_handler.py b/src/tmt_web/utils/git_handler.py index a78a852..1bf569c 100644 --- a/src/tmt_web/utils/git_handler.py +++ b/src/tmt_web/utils/git_handler.py @@ -1,5 +1,4 @@ import contextlib -import os from shutil import rmtree from tmt import Logger @@ -12,6 +11,8 @@ git, ) +from tmt_web import settings + def checkout_branch(path: Path, logger: Logger, ref: str) -> None: """ @@ -20,7 +21,6 @@ def checkout_branch(path: Path, logger: Logger, ref: str) -> None: :param ref: Name of the ref to check out :param path: Path to the repository :param logger: Instance of Logger - :return: """ try: common_instance = Common(logger=logger) @@ -39,7 +39,6 @@ def clone_repository(url: str, logger: Logger, ref: str | None = None) -> None: :param url: URL to the repository :param logger: Instance of Logger :param ref: Optional name of the ref to check out - :return: """ logger.print("Cloning the repository...") path = get_path_to_repository(url) @@ -80,7 +79,7 @@ def get_path_to_repository(url: str) -> Path: """ repo_name = url.rstrip("/").rsplit("/", 1)[-1] root_dir = Path(__file__).resolve().parents[2] # going up from tmt_web/utils/git_handler.py - return root_dir / os.getenv("CLONE_DIR_PATH", "./.repos/") / repo_name + return root_dir / settings.CLONE_DIR_PATH / repo_name def check_if_repository_exists(url: str) -> bool: @@ -98,11 +97,10 @@ def clear_tmp_dir(logger: Logger) -> None: Clears the .tmp directory. :param logger: Instance of Logger - :return: """ logger.print("Clearing the .tmp directory...") root_dir = Path(__file__).resolve().parents[2] # going up from tmt_web/utils/git_handler.py - path = root_dir / os.getenv("CLONE_DIR_PATH", "./.repos/") + path = root_dir / settings.CLONE_DIR_PATH try: rmtree(path) except Exception as e: