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

EDEV-20: Catch model deletions in CI/CD #1530

Merged
merged 15 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions .github/scripts/check_migrations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import subprocess
import sys

DEV_BRANCH = "develop"
LIVE_BRANCH = "main"


def get_diff():
"""
Gets the file diff between the current branch (your branch) and the
develop branch. If there are no changes, it gets the diff between
the current branch and the main branch - this will be the case for
merging develop into the main branch.
"""
subprocess.run(
["git", "fetch", "origin"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
)
output = [
f"./{file_path}"
for file_path in subprocess.check_output(
["git", "diff", "--name-only", f"origin/{DEV_BRANCH}"]
)
.decode()
.splitlines()
]
if not output:
output = [
f"./{file_path}"
for file_path in subprocess.check_output(
["git", "diff", "--name-only", f"origin/{LIVE_BRANCH}"]
)
.decode()
.splitlines()
]
return output


def check_migration_file(file):
"""
Checks if a migration file contains any of the keywords listed
in the `keywords` list that may be potentially harmful to our
data.

While AlterField is generally safe, it can be harmful if the
field is being changed to a type that does not support the
current data type. For example, changing a CharField to an
IntegerField will result in a loss of data.
"""
with open(file) as f:
contents = f.read()

keywords = ["DeleteModel", "RenameModel", "RemoveField", "AlterField"]
for keyword in keywords:
if keyword in contents and f"# etna:allow{keyword}" not in contents:
print(f"Warning: {file} contains a migration that may cause data loss.")
return True


def main():
file_diff = get_diff()

migration_alert = False
for file in file_diff:
if "/migrations/" in file and file.endswith(".py"):
if check_migration_file(file):
migration_alert = True
if migration_alert:
print("Please review the migrations before pushing, to ensure no loss of data.")
sys.exit(1)


if __name__ == "__main__":
main()
23 changes: 23 additions & 0 deletions .github/workflows/check-migrations.yml
TNA-Allan marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Check Migrations

on:
pull_request:
paths:
- 'etna/*/migrations/**'

jobs:
check-migrations:
name: Check Migrations
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v2

- name: Set up Python
uses: actions/setup-python@v2
with:
python-version: '3.10'
TNA-Allan marked this conversation as resolved.
Show resolved Hide resolved

- name: Check migrations
run: python .github/scripts/check_migrations.py
18 changes: 18 additions & 0 deletions docs/developer-guide/project-conventions.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,24 @@ $ fab test --lint

This will be checked by CI on every commit, so it's a good idea to run this locally before pushing your changes.

### Migrations

We also have CI to check Django migrations, in order to help prevent any potential data issues. The CI will only run on a pull request, if any `/migrations` folders have been changed.

If a migration contains a potentially dangerous operation, the developer should check that the migration is safe to run, and verify they have checked this by adding a comment to the migration file,
in the following format:

```python
# etna:allowDeleteModel
```

The operations that require a comment are `DeleteModel`, `RenameModel`, `RemoveField`, and `AlterField`.

If the comment isn't added, the Github Action will flag that the migration is potentially dangerous, and the pull request will fail. The Github Action's log will tell you which file(s) are failing. The developer will then need to add the comment/check the migration, and push the changes to the pull request.

While this won't entirely stop potential data issues, it will help to catch any potential issues by forcing the developer to check that their migrations are sound, before they
are deployed.

### SASS/CSS and Javascript

The Etna project uses a few tools to improve the consistency and quality of SASS/CSS and JavaScript code:
Expand Down