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

[migration] Fix SQLA granularity conversion in migration c5756bec8b47 #5485

Closed

Conversation

jeffreythewang
Copy link
Contributor

@jeffreythewang jeffreythewang commented Jul 25, 2018

This is an extension of #5135, which I believe is originally associated with #4755.

This converts charts that had the old granularity string saved in form data ("Time Column", "hour", "day", "week", etc.) to use ISO duration ("None", "P1H", "P1D", "P1W", etc.).

@betodealmeida @john-bodley @michellethomas @mistercrunch

@codecov-io
Copy link

codecov-io commented Jul 25, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5485   +/-   ##
=======================================
  Coverage   63.28%   63.28%           
=======================================
  Files         349      349           
  Lines       22121    22121           
  Branches     2457     2457           
=======================================
  Hits        13999    13999           
  Misses       8108     8108           
  Partials       14       14

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 3b6cafc...75a92aa. Read the comment docs.

@john-bodley
Copy link
Member

@jeffreythewang shouldn't this be a new migration rather than an augmentation of the existing one?

@jeffreythewang
Copy link
Contributor Author

jeffreythewang commented Jul 25, 2018

Hmm, I'm not sure exactly what process works best for this. It's a bit tricky, as then people won't be able to cherry pick this migration fix into their internal branches (which is what I intended to do with this one). In other words, upgrades from the iso change up to but not including the new revision will have broken charts.

I assumed changing the old migration makes more sense since those who have already upgraded their database to latest may have already fixed their broken charts manually.

Have we done something similar in previous revisions?

@mistercrunch
Copy link
Member

In theory db migrations should be immutable since some people may have checkpointed after the migration you are correcting. There may be a bit more flexibility on mutating migrations that have not yet been released to pypi, which I believe is the case here.

It's probably easier to just create a new migration.

Side note, I think we need to start tagging commits/PRs with migration as they cannot be cherry-picked and it's important to know that when doing release management.

@jeffreythewang jeffreythewang changed the title Fix SQLA granularity conversion in migration c5756bec8b47 [migration] Fix SQLA granularity conversion in migration c5756bec8b47 Jul 28, 2018
@jeffreythewang
Copy link
Contributor Author

btw @mistercrunch I've updated this to be a new migration.

@stale
Copy link

stale bot commented Apr 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 10, 2019
@stale stale bot closed this Apr 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants