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

Remove excluded_projects & include_all_projects columns #81204

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

ceorourke
Copy link
Member

In preparation for removing the AlertRuleExcludedProjects model (see #81020) we need to first remove the many to many excluded_projects column on the AlertRule model. We can take the opportunity to remove the now unused include_all_projects column. This PR uses the brand new SafeRemoveField option added in #81098

@ceorourke ceorourke requested review from a team as code owners November 22, 2024 19:41
@ceorourke ceorourke requested a review from wedamija November 22, 2024 19:41
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 22, 2024
Copy link
Contributor

github-actions bot commented Nov 22, 2024

This PR has a migration; here is the generated SQL for src/sentry/migrations/0794_rm_excluded_included_projects_alertrule.py ()

--
-- Moved alertrule.excluded_projects field to pending deletion state
--
-- (no-op)
--
-- Alter field include_all_projects on alertrule
--
ALTER TABLE "sentry_alertrule" ALTER COLUMN "include_all_projects" DROP NOT NULL;
--
-- Moved alertrule.include_all_projects field to pending deletion state
--
-- (no-op)

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm assuming the AlertRuleExcludedProjects will be removed separately.

@ceorourke
Copy link
Member Author

Looks good to me. I'm assuming the AlertRuleExcludedProjects will be removed separately.

Yep, I'll be removing that and AlertRuleTriggerExclusion next.

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #81204   +/-   ##
=======================================
  Coverage   80.34%   80.34%           
=======================================
  Files        7216     7218    +2     
  Lines      319481   319524   +43     
  Branches    20779    20779           
=======================================
+ Hits       256679   256725   +46     
+ Misses      62408    62405    -3     
  Partials      394      394           

Comment on lines +40 to +44
SafeRemoveField(
model_name="alertrule",
name="excluded_projects",
deletion_action=DeletionAction.MOVE_TO_PENDING,
),
Copy link
Member

Choose a reason for hiding this comment

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

🥳

@ceorourke ceorourke merged commit 59f5d94 into master Nov 25, 2024
50 checks passed
@ceorourke ceorourke deleted the ceorourke/rm-include-exclude-projects branch November 25, 2024 17:15
evanh pushed a commit that referenced this pull request Nov 25, 2024
In preparation for removing the `AlertRuleExcludedProjects` model (see
#81020) we need to first remove
the many to many `excluded_projects` column on the `AlertRule` model. We
can take the opportunity to remove the now unused `include_all_projects`
column. This PR uses the brand new `SafeRemoveField` option added in
#81098
ceorourke added a commit that referenced this pull request Nov 25, 2024
Follow up to #81204 to fully
remove `excluded_projects` and `include_all_projects` from the
`AlertRule` model which was blocking removing the
`AlertRuleExcludedProjects` table.
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
In preparation for removing the `AlertRuleExcludedProjects` model (see
#81020) we need to first remove
the many to many `excluded_projects` column on the `AlertRule` model. We
can take the opportunity to remove the now unused `include_all_projects`
column. This PR uses the brand new `SafeRemoveField` option added in
#81098
andrewshie-sentry pushed a commit that referenced this pull request Dec 2, 2024
Follow up to #81204 to fully
remove `excluded_projects` and `include_all_projects` from the
`AlertRule` model which was blocking removing the
`AlertRuleExcludedProjects` table.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants