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

[sip-15] Enabling SIP-15 by default #9017

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jan 24, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This PR enables SIP-15 by default.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

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

REVIEWERS

to: @etr2460 @robdiciuccio @willbarrett

@john-bodley john-bodley force-pushed the john-bodley--enable-sip-15-by-default branch 3 times, most recently from 3dbb2b7 to 2c5f733 Compare January 24, 2020 21:42
@@ -748,7 +748,7 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
#
# Note if no end date for the grace period is specified then the grace period is
# indefinite.
SIP_15_ENABLED = False
SIP_15_ENABLED = True
SIP_15_GRACE_PERIOD_END: Optional[date] = None # exclusive
Copy link
Member

Choose a reason for hiding this comment

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

Should we add in a grace period date here? Maybe 2020-03-31 or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

@etr2460 I feel like indefinite is preferred to having a hard date, it would be troublesome if someone deployed this change and weren't aware that the grace period had already finished and thus this feature would serve as a hard transition.

Copy link
Member Author

@john-bodley john-bodley Jan 24, 2020

Choose a reason for hiding this comment

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

@mistercrunch @willbarrett do we want to take a passive (no end date) or aggressive (end date specified) approach?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not speaking for @mistercrunch , but I would recommend the aggressive approach. That would provide us a milestone for when the flag can be removed.

UPDATING.md Outdated
defaults to True which ensures that for all new SQL charts the time filter will behave
like [start, end). Existing deployments should either disable this feature to keep the
status quo or inform their users of this change prior to enabling the flag. The
`SIP_15_GRACE_PERIOD_END` option provides a mechanism for specifying long long chart
Copy link
Member

Choose a reason for hiding this comment

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

sp nit: for specifying how long chart

@john-bodley john-bodley force-pushed the john-bodley--enable-sip-15-by-default branch from 2c5f733 to dd835cc Compare January 24, 2020 22:16
@john-bodley john-bodley force-pushed the john-bodley--enable-sip-15-by-default branch from dd835cc to 61935f2 Compare January 24, 2020 23:18
@codecov-io
Copy link

Codecov Report

Merging #9017 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9017   +/-   ##
=======================================
  Coverage   59.15%   59.15%           
=======================================
  Files         367      367           
  Lines       11686    11686           
  Branches     2866     2866           
=======================================
  Hits         6913     6913           
  Misses       4594     4594           
  Partials      179      179

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 f079b6d...61935f2. Read the comment docs.

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

this lgtm regardless of if the grace period is defaulted or not, but ping @mistercrunch and @willbarrett for their thoughts

@john-bodley
Copy link
Member Author

I think for now the default should have SIP-15 enabled which will mean by default an infinite grace period. At a later date we can decide when we want to deprecate the old logic and thus hard-code a transition date.

@john-bodley john-bodley merged commit 848c565 into apache:master Feb 3, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 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 size/XS 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants