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

docs: deprecate old alerts and dash/charts reports #13440

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Mar 3, 2021

SUMMARY

Deprecates the old alerts and reports feature

  • Adds deprecation comments to the config keys
  • Adds deprecation comments on the code modules
  • Adds deprecation log warnings when the feature is enabled
  • Adds a flash message on the MVC list for all the 3 views

Screenshot 2021-03-03 at 16 38 14

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@dpgaspar dpgaspar changed the title docs: deprecated old alerts and dash/charts reports docs: deprecate old alerts and dash/charts reports Mar 3, 2021
@@ -614,18 +615,8 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
CELERY_ACKS_LATE = False
CELERY_ANNOTATIONS = {
"sql_lab.get_sql_results": {"rate_limit": "100/s"},
Copy link
Member

Choose a reason for hiding this comment

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

I think these removals break SEMVER - we shouldn't change the defaults for the configuration between majors. Let's keep the schedules, but perhaps add a deprecation?

EMAIL_ASYNC_TIME_LIMIT_SEC = 300

# Email report configuration
# From address in emails
EMAIL_REPORT_FROM_ADDRESS = "reports@superset.org"
Copy link
Member

Choose a reason for hiding this comment

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

Same comment with these config removals.

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is used anywhere, safe to just remove

Copy link
Member

@willbarrett willbarrett left a comment

Choose a reason for hiding this comment

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

LGTM after we update to conform to SEMVER

@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #13440 (8e58017) into master (26b75fa) will decrease coverage by 0.97%.
The diff coverage is 42.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13440      +/-   ##
==========================================
- Coverage   72.69%   71.71%   -0.98%     
==========================================
  Files         903      804      -99     
  Lines       45611    40651    -4960     
  Branches     5501     4159    -1342     
==========================================
- Hits        33156    29153    -4003     
+ Misses      12243    11498     -745     
+ Partials      212        0     -212     
Flag Coverage Δ
cypress 57.94% <31.21%> (?)
hive 80.00% <83.01%> (+0.02%) ⬆️
javascript ?
mysql 80.34% <83.01%> (+0.01%) ⬆️
postgres ?
presto 80.04% <83.01%> (+0.02%) ⬆️
python 80.81% <83.01%> (-0.06%) ⬇️
sqlite ?

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

Impacted Files Coverage Δ
...erset-frontend/src/SqlLab/components/SouthPane.jsx 64.10% <ø> (-17.95%) ⬇️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 54.02% <ø> (-0.61%) ⬇️
...src/components/FilterableTable/FilterableTable.tsx 3.14% <0.00%> (-79.13%) ⬇️
...et-frontend/src/components/TableSelector/index.tsx 37.16% <ø> (ø)
...hboard/components/menu/BackgroundStyleDropdown.tsx 0.00% <0.00%> (ø)
...dashboard/components/menu/MarkdownModeDropdown.tsx 100.00% <ø> (ø)
...perset-frontend/src/explore/components/Control.tsx 100.00% <ø> (+23.80%) ⬆️
superset/app.py 80.91% <0.00%> (-0.58%) ⬇️
superset/charts/api.py 81.30% <ø> (ø)
superset/databases/api.py 91.93% <ø> (ø)
... and 558 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 26b75fa...f71d5bf. Read the comment docs.

@willbarrett
Copy link
Member

@dpgaspar looks like this just needs a lint fix.

@dpgaspar dpgaspar merged commit 94fc5d5 into apache:master Mar 5, 2021
@dpgaspar dpgaspar deleted the danielgaspar/ch6091/add-additional-deprecation-notices-to-the branch March 5, 2021 09:40
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
* docs: deprecated old alerts and dash/charts reports

* revert celery default config

* fix lint
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/L 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants