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

feat: notify_error default=True for repos created after certain date #320

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

joseph-sentry
Copy link
Contributor

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.

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.
@joseph-sentry joseph-sentry requested a review from a team August 8, 2024 21:09
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.16%. Comparing base (355b4c3) to head (38444be).
Report is 12 commits behind head on main.

Current head 38444be differs from pull request most recent head 96f270e

Please upload reports for the commit 96f270e to get more accurate results.

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     
Flag Coverage Δ
shared-docker-uploader ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@Swatinem Swatinem left a 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.

Comment on lines 10 to 11
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;
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@joseph-sentry joseph-sentry requested a review from Swatinem August 14, 2024 15:50
Copy link
Contributor

@Swatinem Swatinem left a 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;'
Copy link
Contributor

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 NULLable.

@joseph-sentry
Copy link
Contributor Author

Is the intention that every row has a sensible value, or does the NULL have the meaning of "was added before time X"?

null means it must have been added before the date

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.

2 participants