Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a standard pre-commit hook to require safe attribute #39

Closed
tim-schilling opened this issue Sep 8, 2023 · 1 comment · Fixed by #40
Closed

Create a standard pre-commit hook to require safe attribute #39

tim-schilling opened this issue Sep 8, 2023 · 1 comment · Fixed by #40

Comments

@tim-schilling
Copy link
Collaborator

We wrote a simple regex pre-commit hook that's extremely crude. It works well enough for us, but the library should probably have something a bit better.

"""
Search our application's migrations are using django_safemigrate.

This is fairly rudimentary and won't work if the class doesn't explicitly inherit
from ``Migration``.
"""

import os
import re
import sys

MIGRATION_PATTERN = re.compile(r"class\s+(?P<MigrationClass>\w+)\s?\(.*Migration\):")
MIGRATION_FILE_PATTERN = re.compile(r"^\d{4}_.+\.py$")

MISSING_SAFE_MESSAGE = (
    "{file_path}: {migration_class} is missing the 'safe' attribute.\n"
)
FAILURE_MESSAGE = (
    "\n"
    "Add the following to the migration class:\n"
    "from django_safemigrate import Safe"
    "class Migration(migrations.Migration):\n"
    "    safe = Safe.before_deploy\n"
    "\n"
    "You can also use the following:\n"
    "    safe = Safe.always\n"
    "    safe = Safe.after_deploy\n"
)


def find_migration_files(directory):
    for root, _, files in os.walk(directory):
        for file in files:
            file_is_migration = MIGRATION_FILE_PATTERN.match(file)
            if file_is_migration and file.endswith(".py"):
                yield os.path.join(root, file)


def validate_migrations():
    success = True
    for file_path in find_migration_files("."):
        # Strip the leading ./ it's a byproduct of os.walk on the current directory
        file_path = file_path[2:] if file_path.startswith("./") else file_path
        with open(file_path, "r") as f:
            content = f.read()

        match = MIGRATION_PATTERN.search(content)
        if match:
            migration_class = match.group("MigrationClass")
            if "safe = Safe." not in content:
                success = False
                sys.stdout.write(
                    MISSING_SAFE_MESSAGE.format(
                        file_path=file_path, migration_class=migration_class
                    )
                )
    if not success:
        sys.stdout.write(FAILURE_MESSAGE)
    return success


if __name__ == "__main__":
    if not validate_migrations():
        sys.exit(1)

Then in the yaml:

repos:
  - repo: local
    hooks:
    -   id: require-safe-migrations
        name: Require migrations to use django_safemigrate
        entry: python -m devops.linters.require_safe_migrations
        language: system
        pass_filenames: false
@ryanhiebert
Copy link
Owner

This is great, and I think that this would be worthwhile to include even in its crude state. It's sufficient for my purposes. If people find it too finnicky, we'll be happy for contributions to fix that up, otherwise it'll solve a very common problem in an obvious way.

ryanhiebert added a commit that referenced this issue Sep 12, 2023
This pre-commit hook checks the migration files in the current
repository and ensures that they all have a `safe` property
defined. While we can think of easy ways to be break this, for
all the use-cases I've had this covers it quite nicely, so I
think it's worth merging even if we think it's a bit rough.

Tim Schilling wrote this in
#39
and I'm very happy to take that almost verbatim and prove it.
The only thing I changed was to make a new function to handle
sys.exit(), so that I could set it as a console_script to use
with pre-commit.
ryanhiebert added a commit that referenced this issue Sep 12, 2023
This pre-commit hook checks the migration files in the current
repository and ensures that they all have a `safe` property
defined. While we can think of easy ways to be break this, for
all the use-cases I've had this covers it quite nicely, so I
think it's worth merging even if we think it's a bit rough.

Tim Schilling wrote this in
#39
and I'm very happy to take that almost verbatim and prove it.
The only thing I changed was to make a new function to handle
sys.exit(), so that I could set it as a console_script to use
with pre-commit.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
ryanhiebert added a commit that referenced this issue Sep 13, 2023
* Add pre-commit hook

Check the migrations files in the current repository and
ensure that they all have a `safe` property defined.
While we can think of easy ways to break this implementation,
for all the use-cases I've had this covers it quite nicely
so I think it's worth merging even if its rough.

Tim Schilling put an initial sketch of this in
#39
and I was able to modify it for use in a precommit hook
without difficulty. The modifications were made to tailor
it to the environment of pre-commit, where file names are
passed to the command as arguments. This allows pre-commit
to only need to run the checks incrementally for the affected
files for new commits.

Co-authored-by: Tim Schilling <schillingt@better-simple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants