You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/18 17:51:57 UTC
[GitHub] [superset] betodealmeida opened a new pull request #13216: fix: add missing columns
betodealmeida opened a new pull request #13216:
URL: https://github.com/apache/superset/pull/13216
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The migration script `b56500de1855_add_uuid_column_to_import_mixin.py` has a try/except block when adding the `uuid` column to tables; if the operation fails the script might succeed depending on the error. Because of this, people who have ran the migration are having problems running Superset because of missing columns.
I fixed the script by removing the try/except block, and adding a new script that adds the column wherever necessary.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
N/A
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
1. Ran migration, nothing happened as expected (since all my tables have the uuid column)
2. Downgraded to 070c043f2fdb and deleted the `uuid` column from the `dashboards` table
3. Ran migration, and verified that `uuid` was added to `dashboards` and populated.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] ktmud commented on pull request #13216: fix: add missing columns
Posted by GitBox <gi...@apache.org>.
ktmud commented on pull request #13216:
URL: https://github.com/apache/superset/pull/13216#issuecomment-781625391
> The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.
I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida merged pull request #13216: fix: add missing columns
Posted by GitBox <gi...@apache.org>.
betodealmeida merged pull request #13216:
URL: https://github.com/apache/superset/pull/13216
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida edited a comment on pull request #13216: fix: add missing columns
Posted by GitBox <gi...@apache.org>.
betodealmeida edited a comment on pull request #13216:
URL: https://github.com/apache/superset/pull/13216#issuecomment-781593465
> LGTM. Was this user-reported? If so, do you mind linking the GitHub issue if it exists?
Good call. I saw the reports on Slack and Stackoverflow, but there's also a ticket which I just added.
> I think another way to do this is do the column check in `add_uuid_column_to_import_mixin.py`, then simply re-run the same `upgrade` again in `add_missing_uuid_column.py`. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.
The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on pull request #13216: fix: add missing columns
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #13216:
URL: https://github.com/apache/superset/pull/13216#issuecomment-781638632
> > The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.
>
> I was proposing adding the new script, but it rerun the modified code that ignores existing uuid columns. Users don't have to downgrade because it's OK to rerun the same upgrade script when it can ignore existing uuid columns.
Ah, that makes sense. Sorry I misunderstood.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on pull request #13216: fix: add missing columns
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on pull request #13216:
URL: https://github.com/apache/superset/pull/13216#issuecomment-781593465
> I think another way to do this is do the column check in `add_uuid_column_to_import_mixin.py`, then simply re-run the same `upgrade` again in `add_missing_uuid_column.py`. This way you avoid the maximum amount of duplicate code, but I guess current approach works, too.
The problem of modifying the original script is that it forces people to downgrade and upgrade again to fix their database, while adding a new script they just need to upgrade.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org