-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: notify_error default=True for repos created after certain date #320
base: main
Are you sure you want to change the base?
Conversation
Make config option codecov.notify.notify_error default to True for repos created after a certain date. We don't want to change the behaviour for existing repos but we want this to be default behaviour for repos going forward.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #320 +/- ##
==========================================
- Coverage 89.96% 89.16% -0.81%
==========================================
Files 372 324 -48
Lines 11642 9886 -1756
Branches 2083 1771 -312
==========================================
- Hits 10474 8815 -1659
+ Misses 1083 1003 -80
+ Partials 85 68 -17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m no expert in terms of django migrations, but this default logic seems very weird to me.
ALTER TABLE "repos" ADD COLUMN "created_at" timestamp with time zone DEFAULT '2024-08-08T21:17:39.913910+00:00'::timestamptz NULL; | ||
ALTER TABLE "repos" ALTER COLUMN "created_at" DROP DEFAULT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds the column with a default, just to drop that default immediately?
As this column is defined as NULL
, shouldn’t that be the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a really good question. At the moment this will set the value of this column to the default value for all existing rows in the table. I think I will modify this migration to be run such that the command sets the default to null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this reminds me a bit of my own migration in https://github.com/getsentry/sentry/blob/a85dd406e127a3a11625e5b335ca58c46799dee7/src/sentry/migrations/0505_debugfile_date_accessed.py#L30-L40
Is the intention that every row has a sensible value, or does the NULL
have the meaning of "was added before time X"?
|
||
operations = [ | ||
migrations.RunSQL( | ||
'ALTER TABLE "repos" ADD COLUMN "created_at" timestamp with time zone DEFAULT null NULL;' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think you have to define any DEFAULT
if the column is defined as NULL
able.
null means it must have been added before the date |
Make config option codecov.notify.notify_error default to True for repos created after a certain date. We don't want to change the behaviour for existing repos but we want this to be default behaviour for repos going forward.