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

Django Migration Linter Integration #43

Merged
merged 59 commits into from
Jan 14, 2025

Conversation

sanjeevz3009
Copy link
Contributor

@sanjeevz3009 sanjeevz3009 commented Nov 26, 2024

What is the context of this PR?

django-migration-linter was added to the project to automatically identify potentially breaking changes in django migrations files.

The linter will also run in the CI pipeline to provide visibility on migration issues early in the development workflow.

There are two different ways of running the django-linter-migration:

  • One way is the default django-linter-migration command: make lint-migrations.
  • Another way is a custom script that provides a better output when the django-linter-migration tool runs. Locally the developer can choose what command they want to run.: make lint-migrations-custom.

The commands can be found in the Makefile: https://github.com/ONSdigital/dis-wagtail/pull/43/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R53.

The CI pipeline will run the custom output script for better readability in the CI pipeline where there are errors that arise.

Changes made:

  • cms/settings/base.py, poetry.lock and pyproject.toml updated to integrate the linter to the project.
  • ci.yml updated to include linter in CI lint action.
  • Added make commands.
  • Added documentation in the README.
  • Custom script added.

How to review

Set up the project as normal and follow the django-migration-linter section.

Output for make lint-migrations will list out the migrations and have a summary at the end. Such as:

$ python manage.py lintmigrations

(app_add_not_null_column, 0001_create_table)... OK
(app_add_not_null_column, 0002_add_new_not_null_field)... ERR
        NOT NULL constraint on columns
(app_drop_table, 0001_initial)... OK
(app_drop_table, 0002_delete_a)... ERR
        DROPPING table
(app_ignore_migration, 0001_initial)... OK
(app_ignore_migration, 0002_ignore_migration)... IGNORE
(app_rename_table, 0001_initial)... OK
(app_rename_table, 0002_auto_20190414_1500)... ERR
        RENAMING tables

*** Summary ***
Valid migrations: 4/8
Erroneous migrations: 3/8
Migrations with warnings: 0/8
Ignored migrations: 1/8

Output for make lint-migrations-custom will have the list of apps linted, with each app having its migration checked and a summary for it. Such as :

Apps we are linting: 
 ['home', 'core', 'analysis', 'images', 'release_calendar', 'standard_pages', 'users', 'documents', 'themes', 'topics', 'bundles']

Running lintmigrations for documents
(documents, 0001_initial)... WARNING
        CREATE INDEX prolongs transaction, delaying lock release
(documents, 0002_alter_customdocument_file_size)... ERR
        ALTERING columns (Could be backward compatible. You may ignore this migration.) (table: documents_customdocument, column: file_size)
*** Summary ***
Valid migrations: 0/2
Erroneous migrations: 1/2
Migrations with warnings: 1/2
Ignored migrations: 0/2

Follow-up Actions

n/a

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@zerolab
Copy link
Contributor

zerolab commented Nov 28, 2024

There are conflict with the main branch and I suspect this may be the reason the CI is not starting. Best to get this branch up to date with main. Will require sorting out the conflicts in pyproject.toml, and poetry.lock. With the latter, I usually remove it and do poetry lock

Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of suggestions around the custom script

scripts/lintmigrations_custom.py Outdated Show resolved Hide resolved
scripts/lintmigrations_custom.py Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
nehakerung and others added 4 commits December 11, 2024 16:58
* Fix cookiebanner settings link URL variable name (CMS-265)
* Make the contact details name + email unique (CMS-225)
  * Add test for the unique constraint
  * Update contact details listings to include email/phone
* Bump deps. Mainly Django 5.1.4
# Conflicts:
#	poetry.lock
cms/settings/dev.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zerolab zerolab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final question from me (sorry!)

.docker/bashrc.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script was a nice addition. Let's see how this goes. If we find it hard to use and read, it's worth revisiting.

@nehakerung nehakerung merged commit 0ecb45c into main Jan 14, 2025
9 checks passed
@nehakerung nehakerung deleted the feature/django-migration-linter-CMS-201 branch January 14, 2025 15:53
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 this pull request may close these issues.

6 participants