Skip to content
This repository was archived by the owner on May 5, 2025. It is now read-only.

Update repo model with tests enabled #228

Merged
merged 8 commits into from
May 27, 2024
Merged

Update repo model with tests enabled #228

merged 8 commits into from
May 27, 2024

Conversation

RulaKhaled
Copy link
Contributor

Needed to conditionally render onboarding for failed tests.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link

codecov bot commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.48%. Comparing base (7a7847a) to head (4566888).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
- Coverage   89.53%   89.48%   -0.06%     
==========================================
  Files         326      324       -2     
  Lines       10437    10375      -62     
  Branches     1908     1904       -4     
==========================================
- Hits         9345     9284      -61     
+ Misses       1023     1020       -3     
- Partials       69       71       +2     
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.

@RulaKhaled RulaKhaled requested a review from adrian-codecov May 27, 2024 12:36
]

operations = [
migrations.AddField(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the RiskyAddField for this since its updating the repos table

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, anything we add to repo is a risky add?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally yes (similar with other widely used tables like owners and commits) because the migration will timeout because it will be blocked for a long time by other operations touching the table, but if your new column is (default=False, null=True) then in theory it should be ok. But I'm not sure that's always the case and past migrations have gotten me paranoid. So IMO just be safe and do risky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case, the "new" way we get these into prod is by merging this code and then updating the shared version in api/worker. When you do so, schedule sometime with the infra team to run the migration after hours, cause if you merge the code itself, the migrations won't be applied, so they need to run the SQL for you then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you both!

JerrySentry
JerrySentry previously approved these changes May 27, 2024
adrian-codecov
adrian-codecov previously approved these changes May 27, 2024
@RulaKhaled RulaKhaled dismissed stale reviews from adrian-codecov and JerrySentry via 5ba1dea May 27, 2024 17:51
@RulaKhaled RulaKhaled added this pull request to the merge queue May 27, 2024
Merged via the queue into main with commit 5951c1e May 27, 2024
7 checks passed
@RulaKhaled RulaKhaled deleted the tests_enabled branch May 27, 2024 18:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants