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.

Sign up for GitHub

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

Jump to bottom

chore: remove old alerts and configs keys #19261

Merged
merged 14 commits into from
Mar 25, 2022
Merged

chore: remove old alerts and configs keys #19261

merged 14 commits into from
Mar 25, 2022

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Mar 18, 2022 •
edited by eschutho
Loading

SUMMARY

This pr removes the old deprecated Legacy Alerts Code and Config Keys for the coming 2.0 release.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

User should not be able to use or set old alerts.

ADDITIONAL INFORMATION

  • Has associated issue: [2.0] remove legacy ALERTS code and config keys #18689
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@pkdotson pkdotson added the risk:breaking-change Issues or PRs that will introduce breaking changes label Mar 18, 2022
@pkdotson pkdotson marked this pull request as draft March 18, 2022 22:56
@pkdotson pkdotson marked this pull request as ready for review March 23, 2022 03:26
@pkdotson pkdotson added v2.0 need:qa-review Requires QA review preset:QA Preset QA team and removed preset-io labels Mar 23, 2022
@codecov
Copy link

codecov bot commented Mar 23, 2022 •
edited
Loading

Codecov Report

Merging #19261 (a0c1e39) into master (f341025) will decrease coverage by 0.24%.
The diff coverage is 81.04%.

❗ Current head a0c1e39 differs from pull request most recent head d97d386. Consider uploading reports for the commit d97d386 to get more accurate results

@@            Coverage Diff             @@
##           master   #19261      +/-   ##
==========================================
- Coverage   66.53%   66.29%   -0.25%     
==========================================
  Files        1667     1670       +3     
  Lines       64361    63952     -409     
  Branches     6493     6506      +13     
==========================================
- Hits        42825    42399     -426     
- Misses      19854    19867      +13     
- Partials     1682     1686       +4
Flag Coverage Δ
hive ?
mysql 81.65% <90.84%> (+0.16%) ⬆️
postgres 81.70% <90.84%> (+0.16%) ⬆️
presto ?
python 81.74% <90.84%> (-0.21%) ⬇️
sqlite ?

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

Impacted Files Coverage Δ
...erset-ui-chart-controls/src/components/Tooltip.tsx 80.00% <ø> (ø)
...et-ui-chart-controls/src/shared-controls/index.tsx 36.19% <0.00%> (-0.71%) ⬇️
...perset-ui-chart-controls/src/utils/D3Formatting.ts 100.00% <ø> (ø)
...ntend/packages/superset-ui-core/src/color/index.ts 100.00% <ø> (ø)
...s/superset-ui-core/src/query/api/v1/handleError.ts 100.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
.../superset-ui-core/src/query/types/QueryFormData.ts 100.00% <ø> (ø)
...tend/packages/superset-ui-core/src/style/index.tsx 100.00% <ø> (ø)
...end/plugins/legacy-plugin-chart-chord/src/Chord.js 0.00% <0.00%> (ø)
...ns/legacy-plugin-chart-chord/src/transformProps.js 0.00% <0.00%> (ø)
... and 225 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f341025...d97d386. Read the comment docs.

@pkdotson pkdotson changed the title chore(WIP): Remove old alerts chore(): Remove old alerts and configs keys Mar 23, 2022
@pkdotson pkdotson changed the title chore(): Remove old alerts and configs keys chore(): remove old alerts and configs keys Mar 23, 2022
@pkdotson pkdotson changed the title chore(): remove old alerts and configs keys chore: remove old alerts and configs keys Mar 23, 2022
@betodealmeida
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@betodealmeida Ephemeral environment spinning up at http://35.166.1.54:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

betodealmeida
betodealmeida reviewed Mar 23, 2022
View reviewed changes
Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

I love all the red in this PR!

superset/views/alerts.py
@@ -109,144 +55,3 @@ def log(self, pk: int) -> FlaskResponse: # pylint: disable=unused-argument
class AlertView(BaseAlertReportView):
route_base = "/alert"
class_permission_name = "ReportSchedule"
Copy link
Member

Choose a reason for hiding this comment

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

Are these 2 classes still being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes these are used for the dropdown menu alerts and report link

superset/views/alerts.py Outdated Show resolved Hide resolved
@betodealmeida
Copy link
Member

/testenv up FEATURE_ALERT_REPORTS=True

@github-actions
Copy link
Contributor

@betodealmeida Ephemeral environment spinning up at http://54.186.96.55:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@pkdotson pkdotson changed the title chore: remove old alerts and configs keys [WIP]chore: remove old alerts and configs keys Mar 24, 2022
@pkdotson pkdotson changed the title [WIP]chore: remove old alerts and configs keys chore: remove old alerts and configs keys Mar 24, 2022
@hughhhh
Copy link
Member

hughhhh commented Mar 25, 2022

/testenv up FEATURE_ALERT_REPORTS=True

@github-actions
Copy link
Contributor

@hughhhh Ephemeral environment spinning up at http://35.87.219.244:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@hughhhh hughhhh merged commit 394c9a1 into apache:master Mar 25, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@pkdotson @philipher29
chore: remove old alerts and configs keys (apache#19261)
2238ce0 
* remove templates

* remove models and more templates

* remove view

* remove tasks

* remove views

* remove schedule models

* remove views, init files config code

* remove supersetapp init code

* remove tests and clean up pylint errors

* remove unused import

* pylint

* run black

* remove deprecate notice
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betodealmeida betodealmeida betodealmeida left review comments

@hughhhh hughhhh hughhhh approved these changes

@bkyryliuk bkyryliuk Awaiting requested review from bkyryliuk

@eschutho eschutho Awaiting requested review from eschutho

@suddjian suddjian Awaiting requested review from suddjian

@AAfghahi AAfghahi Awaiting requested review from AAfghahi

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review preset:QA Preset QA team risk:breaking-change Issues or PRs that will introduce breaking changes size/XXL 🚢 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pkdotson @betodealmeida @hughhhh @mistercrunch @michael-s-molina