Skip to content

Commit

Permalink
chore: patch recent migration files that drop db columns (#5277)
Browse files Browse the repository at this point in the history
# What this PR does

- patch recent migration files which drop several deprecated DB columns
which led to a recent (minor/internal) issue
- add a CI job to prevent this from happening in the future and instead,
force folks to drop columns [the _proper_
way](https://github.com/grafana/oncall/tree/dev/dev#removing-a-nullable-field-from-a-model)
(which we have documented internally)


(as documented
[here](https://github.com/grafana/oncall/tree/dev/dev#removing-a-nullable-field-from-a-model),
I have the four additional migration files (which actually do the `DROP
COLUMN`s in the db) saved locally, and will include these in a separate
PR/release)

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [x] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] Added the relevant release notes label (see labels prefixed w/
`release:`). These labels dictate how your PR will
    show up in the autogenerated release notes.
  • Loading branch information
joeyorlando authored Nov 20, 2024
1 parent 6334763 commit dd65732
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 16 deletions.
53 changes: 53 additions & 0 deletions .github/workflows/linting-and-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,59 @@ jobs:
python manage.py makemigrations --check
python manage.py lintmigrations
# the following CI check is to prevent developers from dropping columns in a way that could cause downtime
# (the proper way to drop columns is documented in dev/README.md)
#
# we've been bitten by this before (see https://raintank-corp.slack.com/archives/C081TNWM73N as an example)
ensure-database-migrations-drop-columns-the-correct-way:
name: "Ensure database migrations drop columns the correct way"
runs-on: ubuntu-latest
steps:
- name: Checkout PR code
uses: actions/checkout@v3
with:
# Fetch all history so we can compare with the base branch
fetch-depth: 0
# Checkout the head commit of the PR
ref: ${{ github.event.pull_request.head.sha }}

- name: Fetch base branch
run: git fetch origin ${{ github.event.pull_request.base.ref }}:${{ github.event.pull_request.base.ref }}

- name: Check for RemoveField in Migrations
# yamllint disable rule:line-length
run: |
# Get the list of files changed in the PR
git diff --name-only ${{ github.event.pull_request.base.ref }}...${{ github.event.pull_request.head.sha }} > changed_files.txt
# Filter for migration files
grep -E '^.*/migrations/.*\.py$' changed_files.txt > migration_files.txt || true
# Initialize a flag
FAILED=0
# Check each migration file for 'migrations.RemoveField'
if [ -s migration_files.txt ]; then
while IFS= read -r file; do
echo "Checking $file for migrations.RemoveField..."
if grep -q 'migrations.RemoveField' "$file"; then
echo "❌ Error: Found migrations.RemoveField in $file"
FAILED=1
else
echo "✅ No RemoveField found in $file"
fi
done < migration_files.txt
else
echo "No migration files changed."
fi
# Fail the job if RemoveField was found
if [ "$FAILED" -eq 1 ]; then
echo "❌ Error: Found migrations.RemoveField in one or more migration files. Please check out our documentation at https://github.com/grafana/oncall/tree/dev/dev#removing-a-nullable-field-from-a-model on how to properly drop columns."
exit 1
fi
# yamllint enable rule:line-length

unit-test-helm-chart:
name: "Helm Chart Unit Tests"
runs-on: ubuntu-latest-16-cores
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,6 @@ class Migration(migrations.Migration):

operations = [
linter.IgnoreMigration(),
migrations.RemoveField(
model_name='channelfilter',
name='_slack_channel_id',
),
migrations.RemoveField(
model_name='resolutionnoteslackmessage',
name='_slack_channel_id',
),
migrations.DeleteModel(
name='AlertGroupPostmortem',
),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.16 on 2024-11-20 20:21

import common.migrations.remove_field
from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('alerts', '0066_remove_channelfilter__slack_channel_id_and_more'),
]

operations = [
common.migrations.remove_field.RemoveFieldState(
model_name='channelfilter',
name='_slack_channel_id',
),
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.16 on 2024-11-20 20:23

import common.migrations.remove_field
from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('alerts', '0067_remove_channelfilter__slack_channel_id_state'),
]

operations = [
common.migrations.remove_field.RemoveFieldState(
model_name='resolutionnoteslackmessage',
name='_slack_channel_id',
),
]
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Generated by Django 4.2.16 on 2024-11-06 21:13
# Generated by Django 4.2.16 on 2024-11-20 20:12

import common.migrations.remove_field
from django.db import migrations
import django_migration_linter as linter


class Migration(migrations.Migration):
Expand All @@ -11,8 +11,7 @@ class Migration(migrations.Migration):
]

operations = [
linter.IgnoreMigration(),
migrations.RemoveField(
common.migrations.remove_field.RemoveFieldState(
model_name='oncallschedule',
name='channel',
),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# Generated by Django 4.2.16 on 2024-11-06 21:11
# Generated by Django 4.2.16 on 2024-11-20 17:58

import common.migrations.remove_field
from django.db import migrations
import django_migration_linter as linter


class Migration(migrations.Migration):
Expand All @@ -11,8 +11,7 @@ class Migration(migrations.Migration):
]

operations = [
linter.IgnoreMigration(),
migrations.RemoveField(
common.migrations.remove_field.RemoveFieldState(
model_name='organization',
name='general_log_channel_id',
),
Expand Down

0 comments on commit dd65732

Please sign in to comment.