diff --git a/.github/workflows/linting-and-tests.yml b/.github/workflows/linting-and-tests.yml index 23688595e6..d94506638b 100644 --- a/.github/workflows/linting-and-tests.yml +++ b/.github/workflows/linting-and-tests.yml @@ -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 @@ -193,7 +246,7 @@ jobs: ONCALL_TESTING_RBAC_ENABLED: ${{ matrix.rbac_enabled }} services: redis_test: - image: redis:7.0.5 + image: redis:7.0.15 ports: - 6379:6379 options: >- diff --git a/dev/scripts/generate-fake-data/requirements.txt b/dev/scripts/generate-fake-data/requirements.txt index 74976b8c29..29b74d6b8a 100644 --- a/dev/scripts/generate-fake-data/requirements.txt +++ b/dev/scripts/generate-fake-data/requirements.txt @@ -1,3 +1,3 @@ -aiohttp==3.10.2 +aiohttp==3.10.11 Faker==16.4.0 tqdm==4.66.3 diff --git a/docker-compose-developer.yml b/docker-compose-developer.yml index ee668df794..7b4143f260 100644 --- a/docker-compose-developer.yml +++ b/docker-compose-developer.yml @@ -172,7 +172,7 @@ services: redis: container_name: redis labels: *oncall-labels - image: redis:7.0.5 + image: redis:7.0.15 restart: always ports: - "6379:6379" diff --git a/docker-compose-mysql-rabbitmq.yml b/docker-compose-mysql-rabbitmq.yml index 60b320e80f..a00d701672 100644 --- a/docker-compose-mysql-rabbitmq.yml +++ b/docker-compose-mysql-rabbitmq.yml @@ -87,7 +87,7 @@ services: retries: 10 redis: - image: redis:7.0.5 + image: redis:7.0.15 restart: always expose: - 6379 diff --git a/docker-compose.yml b/docker-compose.yml index c54c2fb33f..0d687d4e99 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -54,7 +54,7 @@ services: condition: service_healthy redis: - image: redis:7.0.5 + image: redis:7.0.15 restart: always expose: - 6379 diff --git a/engine/apps/alerts/incident_log_builder/incident_log_builder.py b/engine/apps/alerts/incident_log_builder/incident_log_builder.py index 9fb1f0021c..d291d4fade 100644 --- a/engine/apps/alerts/incident_log_builder/incident_log_builder.py +++ b/engine/apps/alerts/incident_log_builder/incident_log_builder.py @@ -152,6 +152,15 @@ def _get_user_notification_log_records_for_log_report(self) -> "RelatedManager[' Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_TRIGGERED) & Q(notification_policy__step=UserNotificationPolicy.Step.WAIT) ) + # Exclude SUCCESS + ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED, these cause confusions as the user + # has already been notified by another path so this step should not be displayed, although it is kept + # for auditing. + | Q( + Q(type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS) + & Q( + notification_error_code=UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_POSTING_TO_SLACK_IS_DISABLED + ) + ) ) .select_related("author") .distinct() diff --git a/engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py b/engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py index 03c5f53430..7c23e789e2 100644 --- a/engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py +++ b/engine/apps/alerts/migrations/0066_remove_channelfilter__slack_channel_id_and_more.py @@ -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', ), diff --git a/engine/apps/alerts/migrations/0067_remove_channelfilter__slack_channel_id_state.py b/engine/apps/alerts/migrations/0067_remove_channelfilter__slack_channel_id_state.py new file mode 100644 index 0000000000..b819201d0c --- /dev/null +++ b/engine/apps/alerts/migrations/0067_remove_channelfilter__slack_channel_id_state.py @@ -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', + ), + ] diff --git a/engine/apps/alerts/migrations/0068_remove_resolutionnoteslackmessage__slack_channel_id_state.py b/engine/apps/alerts/migrations/0068_remove_resolutionnoteslackmessage__slack_channel_id_state.py new file mode 100644 index 0000000000..a270331c73 --- /dev/null +++ b/engine/apps/alerts/migrations/0068_remove_resolutionnoteslackmessage__slack_channel_id_state.py @@ -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', + ), + ] diff --git a/engine/apps/alerts/tasks/notify_user.py b/engine/apps/alerts/tasks/notify_user.py index 88f46f6e9c..2cc8b89e91 100644 --- a/engine/apps/alerts/tasks/notify_user.py +++ b/engine/apps/alerts/tasks/notify_user.py @@ -528,7 +528,7 @@ def perform_notification(log_record_pk, use_default_notification_policy_fallback ) UserNotificationPolicyLogRecord( author=user, - type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED, + type=UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS, notification_policy=notification_policy, reason="Prevented from posting in Slack", alert_group=alert_group, diff --git a/engine/apps/alerts/tests/test_notify_user.py b/engine/apps/alerts/tests/test_notify_user.py index a24003dfe9..2b885cedbe 100644 --- a/engine/apps/alerts/tests/test_notify_user.py +++ b/engine/apps/alerts/tests/test_notify_user.py @@ -315,7 +315,7 @@ def test_perform_notification_slack_prevent_posting( mocked_send_slack_notification.assert_not_called() last_log_record = UserNotificationPolicyLogRecord.objects.last() - assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_FAILED + assert last_log_record.type == UserNotificationPolicyLogRecord.TYPE_PERSONAL_NOTIFICATION_SUCCESS assert last_log_record.reason == "Prevented from posting in Slack" assert ( last_log_record.notification_error_code diff --git a/engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py b/engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py index 155a6161b2..fa0dc3fff4 100644 --- a/engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py +++ b/engine/apps/schedules/ical_events/adapter/amixr_recurring_ical_events_adapter.py @@ -20,6 +20,15 @@ class AmixrRecurringIcalEventsAdapter(IcalService): + def _normalize(self, dt: datetime.datetime) -> datetime.datetime: + tz = getattr(dt, "tzinfo", None) + if tz: + normalized = tz.normalize(dt) + if normalized.tzinfo != tz: + diff = dt.dst() - normalized.dst() + dt = normalized + diff + return dt + def get_events_from_ical_between( self, calendar: Calendar, start_date: datetime.datetime, end_date: datetime.datetime ) -> typing.List[Event]: @@ -38,6 +47,10 @@ def get_events_from_ical_between( end_date + datetime.timedelta(days=EXTRA_LOOKUP_DAYS), ) + for event in events: + # account for timezones not being properly calculated when DST changes. + event[ICAL_DATETIME_END].dt = self._normalize(event[ICAL_DATETIME_END].dt) + def filter_extra_days(event): event_start, event_end = self.get_start_and_end_with_respect_to_event_type(event) if event_start > event_end: diff --git a/engine/apps/schedules/migrations/0020_remove_oncallschedule_channel.py b/engine/apps/schedules/migrations/0020_remove_oncallschedule_channel_state.py similarity index 62% rename from engine/apps/schedules/migrations/0020_remove_oncallschedule_channel.py rename to engine/apps/schedules/migrations/0020_remove_oncallschedule_channel_state.py index e4d1913827..23e3f96f6b 100644 --- a/engine/apps/schedules/migrations/0020_remove_oncallschedule_channel.py +++ b/engine/apps/schedules/migrations/0020_remove_oncallschedule_channel_state.py @@ -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): @@ -11,8 +11,7 @@ class Migration(migrations.Migration): ] operations = [ - linter.IgnoreMigration(), - migrations.RemoveField( + common.migrations.remove_field.RemoveFieldState( model_name='oncallschedule', name='channel', ), diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index e63c96b2f8..71a029b468 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -1818,6 +1818,63 @@ def test_refresh_ical_final_schedule_ok( assert event in expected_events +@pytest.mark.django_db +def test_filter_events_during_dst_change( + make_organization, + make_user_for_organization, + make_schedule, + make_on_call_shift, +): + organization = make_organization() + u1 = make_user_for_organization(organization) + + schedule = make_schedule( + organization, + time_zone="America/Chicago", # UTC-6 or UTC-5 depending on DST + schedule_class=OnCallScheduleCalendar, + ) + start_datetime = timezone.datetime(2024, 10, 1, 0, 0, 0, tzinfo=timezone.utc) + duration = timezone.timedelta(seconds=60 * 60 * 24 * 7) # 1 week + data = { + "start": start_datetime, + "rotation_start": start_datetime, + "duration": duration, + "frequency": CustomOnCallShift.FREQUENCY_WEEKLY, + "week_start": CustomOnCallShift.MONDAY, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[u1]]) + schedule.custom_on_call_shifts.add(on_call_shift) + + schedule.refresh_ical_file() + + # week with DST change + start_date = timezone.datetime(2024, 11, 4, 0, 0, 0, tzinfo=timezone.utc) + end_date = start_date + timezone.timedelta(days=1) + events = schedule.filter_events(start_date, end_date) + expected = { + "start": timezone.datetime(2024, 10, 29, 5, 0, 0, tzinfo=timezone.utc), + "end": timezone.datetime(2024, 11, 5, 6, 0, 0, tzinfo=timezone.utc), + } + assert len(events) == 1 + returned = {"start": events[0]["start"], "end": events[0]["end"]} + assert returned == expected + + # week with DST change back + start_date = timezone.datetime(2025, 3, 10, 0, 0, 0, tzinfo=timezone.utc) + end_date = start_date + timezone.timedelta(days=1) + events = schedule.filter_events(start_date, end_date) + expected = { + "start": timezone.datetime(2025, 3, 4, 6, 0, 0, tzinfo=timezone.utc), + "end": timezone.datetime(2025, 3, 11, 5, 0, 0, tzinfo=timezone.utc), + } + assert len(events) == 1 + returned = {"start": events[0]["start"], "end": events[0]["end"]} + assert returned == expected + + @pytest.mark.django_db def test_refresh_ical_final_schedule_cancel_deleted_events( make_organization, diff --git a/engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id.py b/engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id_state.py similarity index 63% rename from engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id.py rename to engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id_state.py index 6d415bdb44..4e5d61ebe1 100644 --- a/engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id.py +++ b/engine/apps/user_management/migrations/0028_remove_organization_general_log_channel_id_state.py @@ -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): @@ -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', ),