From 3e56f9cfb8ce0477a4d407a0e865405933e56d41 Mon Sep 17 00:00:00 2001 From: "richard.smith" Date: Thu, 7 Sep 2023 16:20:21 +0100 Subject: [PATCH 1/2] harmonise loading between makemigrations and lintmigrations commands --- .../management/commands/lintmigrations.py | 73 +----------- .../management/commands/makemigrations.py | 5 +- django_migration_linter/management/utils.py | 105 ++++++++++++++++++ tests/unit/files/test_config.toml | 2 + tests/unit/files/test_setup.cfg | 2 + tests/unit/test_management_utils.py | 103 +++++++++++++++++ 6 files changed, 218 insertions(+), 72 deletions(-) create mode 100644 tests/unit/files/test_config.toml create mode 100644 tests/unit/files/test_setup.cfg create mode 100644 tests/unit/test_management_utils.py diff --git a/django_migration_linter/management/commands/lintmigrations.py b/django_migration_linter/management/commands/lintmigrations.py index 73170616..78e32c89 100644 --- a/django_migration_linter/management/commands/lintmigrations.py +++ b/django_migration_linter/management/commands/lintmigrations.py @@ -1,32 +1,19 @@ from __future__ import annotations -import configparser -import itertools import os import sys from importlib import import_module -from typing import Any, Callable -import toml -from django.conf import settings from django.core.management.base import BaseCommand, CommandParser from ...constants import __version__ from ...migration_linter import MessageType, MigrationLinter from ..utils import ( - configure_logging, extract_warnings_as_errors_option, + load_config, register_linting_configuration_options, ) -CONFIG_NAME = "django_migration_linter" -PYPROJECT_TOML = "pyproject.toml" -DEFAULT_CONFIG_FILES = ( - f".{CONFIG_NAME}.cfg", - "setup.cfg", - "tox.ini", -) - class Command(BaseCommand): help = "Lint your migrations" @@ -135,24 +122,14 @@ def add_arguments(self, parser: CommandParser) -> None: register_linting_configuration_options(parser) def handle(self, *args, **options): - django_settings_options = self.read_django_settings(options) - config_options = self.read_config_file(options) - toml_options = self.read_toml_file(options) - for k, v in itertools.chain( - django_settings_options.items(), - config_options.items(), - toml_options.items(), - ): - if not options[k]: - options[k] = v + options = load_config(options) + ( warnings_as_errors_tests, all_warnings_as_errors, ) = extract_warnings_as_errors_option(options["warnings_as_errors"]) - configure_logging(options["verbosity"]) - root_path = options["project_root_path"] or os.path.dirname( import_module(os.getenv("DJANGO_SETTINGS_MODULE")).__file__ ) @@ -186,49 +163,5 @@ def handle(self, *args, **options): if linter.has_errors: sys.exit(1) - @staticmethod - def read_django_settings(options: dict[str, Any]) -> dict[str, Any]: - django_settings_options = dict() - - django_migration_linter_settings = getattr( - settings, "MIGRATION_LINTER_OPTIONS", dict() - ) - for key in options: - if key in django_migration_linter_settings: - django_settings_options[key] = django_migration_linter_settings[key] - - return django_settings_options - - @staticmethod - def read_config_file(options: dict[str, Any]) -> dict[str, Any]: - config_options = dict() - - config_parser = configparser.ConfigParser() - config_parser.read(DEFAULT_CONFIG_FILES, encoding="utf-8") - for key, value in options.items(): - config_get_fn: Callable - if isinstance(value, bool): - config_get_fn = config_parser.getboolean - else: - config_get_fn = config_parser.get - - config_value = config_get_fn(CONFIG_NAME, key, fallback=None) - if config_value is not None: - config_options[key] = config_value - return config_options - - @staticmethod - def read_toml_file(options: dict[str, Any]) -> dict[str, Any]: - toml_options = dict() - - if os.path.exists(PYPROJECT_TOML): - pyproject_toml = toml.load(PYPROJECT_TOML) - section = pyproject_toml.get("tool", {}).get(CONFIG_NAME, {}) - for key in options: - if key in section: - toml_options[key] = section[key] - - return toml_options - def get_version(self) -> str: return __version__ diff --git a/django_migration_linter/management/commands/makemigrations.py b/django_migration_linter/management/commands/makemigrations.py index b842c0d2..8ef855a3 100644 --- a/django_migration_linter/management/commands/makemigrations.py +++ b/django_migration_linter/management/commands/makemigrations.py @@ -14,8 +14,8 @@ from django_migration_linter import MigrationLinter from ..utils import ( - configure_logging, extract_warnings_as_errors_option, + load_config, register_linting_configuration_options, ) @@ -47,12 +47,13 @@ def add_arguments(self, parser: CommandParser) -> None: register_linting_configuration_options(parser) def handle(self, *app_labels, **options): + options = load_config(options) + self.lint = options["lint"] self.database = options["database"] self.exclude_migrations_tests = options["exclude_migration_tests"] self.warnings_as_errors = options["warnings_as_errors"] self.sql_analyser = options["sql_analyser"] - configure_logging(options["verbosity"]) return super().handle(*app_labels, **options) def write_migration_files(self, changes: dict[str, list[Migration]]) -> None: diff --git a/django_migration_linter/management/utils.py b/django_migration_linter/management/utils.py index effc29d3..7ef73be8 100644 --- a/django_migration_linter/management/utils.py +++ b/django_migration_linter/management/utils.py @@ -1,12 +1,34 @@ from __future__ import annotations +from typing import Any, Callable +import configparser import logging +import itertools +import os + +import toml from django.core.management import CommandParser +from django.conf import settings from ..sql_analyser.analyser import ANALYSER_STRING_MAPPING +CONFIG_NAME = "django_migration_linter" +PYPROJECT_TOML = "pyproject.toml" +DEFAULT_CONFIG_FILES = ( + f".{CONFIG_NAME}.cfg", + "setup.cfg", + "tox.ini", +) + + +class ConfigCommandParser(CommandParser): + def parse_args(self, args=None, namespace=None): + config_options = load_config() + return super().parse_args(args, namespace) + + def register_linting_configuration_options(parser: CommandParser) -> None: parser.add_argument( "--database", @@ -65,3 +87,86 @@ def extract_warnings_as_errors_option( all_warnings_as_errors = False return warnings_as_errors_tests, all_warnings_as_errors + + +def load_config(options: dict) -> dict[str, Any]: + """ + Load config options and setup logging. + + Settings are loaded in priority order, where higher + priority settings override lower. + + 1. Command line + 2. Django Settings + 3. Config files + 4. pyproject.toml + """ + + django_settings_options = read_django_settings(options) + config_options = read_config_file(options) + toml_options = read_toml_file(options) + for k, v in itertools.chain( + toml_options.items(), + config_options.items(), + django_settings_options.items(), + ): + if not options[k]: + options[k] = v + + configure_logging(options["verbosity"]) + + return options + + +def read_django_settings(options: dict[str, Any]) -> dict[str, Any]: + """ + Read configuration from django settings + """ + django_settings_options = dict() + + django_migration_linter_settings = getattr( + settings, "MIGRATION_LINTER_OPTIONS", dict() + ) + for key in options: + if key in django_migration_linter_settings: + django_settings_options[key] = django_migration_linter_settings[key] + + return django_migration_linter_settings + + +def read_config_file(options: dict[str, Any]) -> dict[str, Any]: + """ + Read config options from any of the supported files specified + in DEFAULT_CONFIG_FILES. + """ + config_options = dict() + + config_parser = configparser.ConfigParser() + config_parser.read(DEFAULT_CONFIG_FILES, encoding="utf-8") + for key, value in options.items(): + config_get_fn: Callable + if isinstance(value, bool): + config_get_fn = config_parser.getboolean + else: + config_get_fn = config_parser.get + + config_value = config_get_fn(CONFIG_NAME, key, fallback=None) + if config_value is not None: + config_options[key] = config_value + return config_options + + +def read_toml_file(options: dict[str, Any]) -> dict[str, Any]: + """ + Read config options from toml file. + """ + toml_options = dict() + + if os.path.exists(PYPROJECT_TOML): + pyproject_toml = toml.load(PYPROJECT_TOML) + section = pyproject_toml.get("tool", {}).get(CONFIG_NAME, {}) + for key in options: + if key in section: + toml_options[key] = section[key] + + return toml_options \ No newline at end of file diff --git a/tests/unit/files/test_config.toml b/tests/unit/files/test_config.toml new file mode 100644 index 00000000..c87a985b --- /dev/null +++ b/tests/unit/files/test_config.toml @@ -0,0 +1,2 @@ +[tool.django_migration_linter] +sql_analyser = "postgresql" diff --git a/tests/unit/files/test_setup.cfg b/tests/unit/files/test_setup.cfg new file mode 100644 index 00000000..723f553a --- /dev/null +++ b/tests/unit/files/test_setup.cfg @@ -0,0 +1,2 @@ +[django_migration_linter] +traceback = True \ No newline at end of file diff --git a/tests/unit/test_management_utils.py b/tests/unit/test_management_utils.py new file mode 100644 index 00000000..dae4a2d6 --- /dev/null +++ b/tests/unit/test_management_utils.py @@ -0,0 +1,103 @@ +import os +from unittest.mock import patch + +import unittest +from django.test import TestCase +from django.test.utils import override_settings + +from django_migration_linter.management import utils + + +EXAMPLE_OPTIONS = { + "verbosity": 1, + "settings": None, + "pythonpath": None, + "traceback": False, + "no_color": False, + "force_color": False, + "skip_checks": False, + "dry_run": False, + "merge": False, + "empty": False, + "interactive": True, + "name": None, + "include_header": True, + "check_changes": False, + "scriptable": False, + "update": False, + "lint": False, + "database": None, + "exclude_migration_tests": None, + "warnings_as_errors": None, + "sql_analyser": None, +} + + +def get_test_path(filename: str) -> str: + """ + Returns path to test files + """ + return os.path.join(os.path.dirname(__file__), f"files/{filename}") + + +class LoadConfigTestCase(unittest.TestCase): + @patch( + "django_migration_linter.management.utils.PYPROJECT_TOML", + get_test_path("test_config.toml"), + ) + def test_read_toml_file_exists(self): + """ + Check we can read config options from a toml file. + """ + toml_options = utils.read_toml_file(EXAMPLE_OPTIONS) + expected = {"sql_analyser": "postgresql"} + self.assertDictEqual(toml_options, expected) + + def test_read_toml_file_dn_exist(self): + """ + Check no errors are raised if the toml file does not exist. + """ + toml_options = utils.read_toml_file(EXAMPLE_OPTIONS) + expected = {} + self.assertDictEqual(toml_options, expected) + + @patch( + "django_migration_linter.management.utils.DEFAULT_CONFIG_FILES", + get_test_path("test_setup.cfg"), + ) + def test_read_config_file_exists(self): + """ + Check we can read config options from a config file + """ + cfg_options = utils.read_config_file(EXAMPLE_OPTIONS) + expected = {"traceback": True} + self.assertDictEqual(cfg_options, expected) + + def test_read_config_file_dn_exist(self): + """ + Check no errors are raised if the .cfg file does not exist. + """ + cfg_options = utils.read_config_file(EXAMPLE_OPTIONS) + expected = {} + self.assertDictEqual(cfg_options, expected) + + # @override_settings( + # MIGRATION_LINTER_OPTIONS={"interactive": False, "name": "test_name"} + # ) + # def test_read_django_settings_exist(self): + # django_options = utils.read_django_settings(EXAMPLE_OPTIONS) + # expected = {"interactive": False, "name": "test_name"} + # self.assertDictEqual(django_options, expected) + + def test_read_django_settings_dn_exist(self): + """ + Check no errors are raised if the settings key does not exist. + """ + django_options = utils.read_django_settings(EXAMPLE_OPTIONS) + expected = {} + self.assertDictEqual(django_options, expected) + + def test_load_config_priority(self): + """ """ + + options = utils.load_config(EXAMPLE_OPTIONS) From 72921ded0fc80a0bd808cedf9be1085a5c8957eb Mon Sep 17 00:00:00 2001 From: "richard.smith" Date: Fri, 8 Sep 2023 11:28:24 +0100 Subject: [PATCH 2/2] sort loading priority --- .../management/commands/lintmigrations.py | 6 +- .../management/commands/makemigrations.py | 7 +- django_migration_linter/management/utils.py | 42 +++++----- docs/usage.md | 4 + tests/unit/files/test_config.toml | 1 + tests/unit/files/test_setup.cfg | 3 +- tests/unit/test_management_utils.py | 80 ++++++++++++++++--- 7 files changed, 105 insertions(+), 38 deletions(-) diff --git a/django_migration_linter/management/commands/lintmigrations.py b/django_migration_linter/management/commands/lintmigrations.py index 78e32c89..88317519 100644 --- a/django_migration_linter/management/commands/lintmigrations.py +++ b/django_migration_linter/management/commands/lintmigrations.py @@ -9,9 +9,10 @@ from ...constants import __version__ from ...migration_linter import MessageType, MigrationLinter from ..utils import ( + configure_logging, extract_warnings_as_errors_option, - load_config, register_linting_configuration_options, + set_defaults_from_conf, ) @@ -120,10 +121,11 @@ def add_arguments(self, parser: CommandParser) -> None: help="don't print linting messages to stdout", ) register_linting_configuration_options(parser) + set_defaults_from_conf(parser) def handle(self, *args, **options): - options = load_config(options) + configure_logging(options["verbosity"]) ( warnings_as_errors_tests, diff --git a/django_migration_linter/management/commands/makemigrations.py b/django_migration_linter/management/commands/makemigrations.py index 8ef855a3..f1c58a39 100644 --- a/django_migration_linter/management/commands/makemigrations.py +++ b/django_migration_linter/management/commands/makemigrations.py @@ -14,9 +14,10 @@ from django_migration_linter import MigrationLinter from ..utils import ( + configure_logging, extract_warnings_as_errors_option, - load_config, register_linting_configuration_options, + set_defaults_from_conf, ) @@ -45,9 +46,11 @@ def add_arguments(self, parser: CommandParser) -> None: help="Lint newly generated migrations.", ) register_linting_configuration_options(parser) + set_defaults_from_conf(parser) + def handle(self, *app_labels, **options): - options = load_config(options) + configure_logging(options["verbosity"]) self.lint = options["lint"] self.database = options["database"] diff --git a/django_migration_linter/management/utils.py b/django_migration_linter/management/utils.py index 7ef73be8..58b11ecc 100644 --- a/django_migration_linter/management/utils.py +++ b/django_migration_linter/management/utils.py @@ -23,12 +23,6 @@ ) -class ConfigCommandParser(CommandParser): - def parse_args(self, args=None, namespace=None): - config_options = load_config() - return super().parse_args(args, namespace) - - def register_linting_configuration_options(parser: CommandParser) -> None: parser.add_argument( "--database", @@ -88,34 +82,34 @@ def extract_warnings_as_errors_option( return warnings_as_errors_tests, all_warnings_as_errors - -def load_config(options: dict) -> dict[str, Any]: +def set_defaults_from_conf(parser) -> None: """ - Load config options and setup logging. + Load options defined in config. Settings are loaded in priority order, where higher priority settings override lower. - 1. Command line - 2. Django Settings - 3. Config files - 4. pyproject.toml + 1. Django Settings + 2. Config files + 3. pyproject.toml + + Any command line options then override config file args. """ - django_settings_options = read_django_settings(options) - config_options = read_config_file(options) + # Parse args to get the relevant options for the current command + args = parser.parse_args() + options = vars(args) + + # Retrieve relevant config values toml_options = read_toml_file(options) - for k, v in itertools.chain( - toml_options.items(), - config_options.items(), - django_settings_options.items(), - ): - if not options[k]: - options[k] = v + config_options = read_config_file(options) + django_settings_options = read_django_settings(options) - configure_logging(options["verbosity"]) + # Merge values from configs in priority order toml -> conf -> settings + conf_options = {k:v for k,v in itertools.chain(toml_options.items(),config_options.items(),django_settings_options.items())} - return options + # Override default argparse arguments with conf options + parser.set_defaults(**conf_options) def read_django_settings(options: dict[str, Any]) -> dict[str, Any]: diff --git a/docs/usage.md b/docs/usage.md index 26a15c0d..e3d880d9 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -99,3 +99,7 @@ The migration test codes can be found in the [corresponding source code files](. [3YOURMIND](https://www.3yourmind.com/) is running the linter on every build getting pushed through CI. That enables to be sure that the migrations will allow A/B testing, Blue/Green deployment, and they won't break your development environment. A non-zero error code is returned to express that at least one invalid migration has been found. + + +## Postgis +You might be able to get away with using the `postgresql` sql analyser option with PostGIS. This can be configured using the `sql_analyser` option. \ No newline at end of file diff --git a/tests/unit/files/test_config.toml b/tests/unit/files/test_config.toml index c87a985b..411568a9 100644 --- a/tests/unit/files/test_config.toml +++ b/tests/unit/files/test_config.toml @@ -1,2 +1,3 @@ [tool.django_migration_linter] sql_analyser = "postgresql" +not_a_real_setting = true \ No newline at end of file diff --git a/tests/unit/files/test_setup.cfg b/tests/unit/files/test_setup.cfg index 723f553a..e34a613b 100644 --- a/tests/unit/files/test_setup.cfg +++ b/tests/unit/files/test_setup.cfg @@ -1,2 +1,3 @@ [django_migration_linter] -traceback = True \ No newline at end of file +traceback = True +not_a_real_setting = true \ No newline at end of file diff --git a/tests/unit/test_management_utils.py b/tests/unit/test_management_utils.py index dae4a2d6..3ba29e73 100644 --- a/tests/unit/test_management_utils.py +++ b/tests/unit/test_management_utils.py @@ -1,5 +1,6 @@ import os from unittest.mock import patch +import argparse import unittest from django.test import TestCase @@ -52,6 +53,18 @@ def test_read_toml_file_exists(self): toml_options = utils.read_toml_file(EXAMPLE_OPTIONS) expected = {"sql_analyser": "postgresql"} self.assertDictEqual(toml_options, expected) + + @patch( + "django_migration_linter.management.utils.PYPROJECT_TOML", + get_test_path("test_config.toml"), + ) + def test_read_toml_file_drops_extras(self): + """ + Check extra items listed in the toml file are dropped + """ + toml_options = utils.read_toml_file(EXAMPLE_OPTIONS) + expected = {"sql_analyser": "postgresql"} + self.assertDictEqual(toml_options, expected) def test_read_toml_file_dn_exist(self): """ @@ -72,6 +85,18 @@ def test_read_config_file_exists(self): cfg_options = utils.read_config_file(EXAMPLE_OPTIONS) expected = {"traceback": True} self.assertDictEqual(cfg_options, expected) + + @patch( + "django_migration_linter.management.utils.DEFAULT_CONFIG_FILES", + get_test_path("test_setup.cfg"), + ) + def test_read_config_file_drop_extra(self): + """ + Check we can read config options from a config file + """ + cfg_options = utils.read_config_file(EXAMPLE_OPTIONS) + expected = {"traceback": True} + self.assertDictEqual(cfg_options, expected) def test_read_config_file_dn_exist(self): """ @@ -81,13 +106,21 @@ def test_read_config_file_dn_exist(self): expected = {} self.assertDictEqual(cfg_options, expected) - # @override_settings( - # MIGRATION_LINTER_OPTIONS={"interactive": False, "name": "test_name"} - # ) - # def test_read_django_settings_exist(self): - # django_options = utils.read_django_settings(EXAMPLE_OPTIONS) - # expected = {"interactive": False, "name": "test_name"} - # self.assertDictEqual(django_options, expected) + @override_settings( + MIGRATION_LINTER_OPTIONS={"interactive": False, "name": "test_name"} + ) + def test_read_django_settings_exist(self): + django_options = utils.read_django_settings(EXAMPLE_OPTIONS) + expected = {"interactive": False, "name": "test_name"} + self.assertDictEqual(django_options, expected) + + @override_settings( + MIGRATION_LINTER_OPTIONS={"interactive": False, "name": "test_name", "not_a_real_setting": True} + ) + def test_read_django_settings_drop_extra(self): + django_options = utils.read_django_settings(EXAMPLE_OPTIONS) + expected = {"interactive": False, "name": "test_name"} + self.assertDictEqual(django_options, expected) def test_read_django_settings_dn_exist(self): """ @@ -97,7 +130,36 @@ def test_read_django_settings_dn_exist(self): expected = {} self.assertDictEqual(django_options, expected) + @patch("django_migration_linter.management.utils.read_django_settings", lambda options: {"a": 2}) + @patch("django_migration_linter.management.utils.read_config_file", lambda options: {"b": "3"}) + @patch("django_migration_linter.management.utils.read_toml_file", lambda options: {"a": 1,"b": 2, "c": "4", "d": False}) def test_load_config_priority(self): - """ """ + """ + Check that the loading priority is respected. + + Expecting: + a is overridden by django settings + b is overridden by cfg file + c is loaded from toml and converted to int + d is overridden command line + """ + + parser = argparse.ArgumentParser() + parser.add_argument("--a", default=1) + parser.add_argument("--b", type=int, default=1) + parser.add_argument("--c", type=int, default=1) + parser.add_argument("--d", action="store_true") + + with patch('argparse._sys.argv', ["script.py"]): + utils.set_defaults_from_conf(parser) + + expected = { + "a": 2, + "b": 3, + "c": 4, + "d": True + } + + options = vars(parser.parse_args(["--d"])) - options = utils.load_config(EXAMPLE_OPTIONS) + self.assertDictEqual(options, expected)