-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
feat: Adding configuration_method column to Database Model #14433
Conversation
I only see a single migration file. Where is the enum defined, etc? 🤔 |
Hey @craig-rueda! I am working on all of that in another PR, didn't want to add anything more to a migration. I'll send you a link when it is done! |
Why not just combine the two? That keeps your relevant changes together, making things easier to reason about, test, etc. |
❗ Please consider rebasing your branch to avoid db migration conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craig-rueda we've been doing the migration scripts on separate PRs because it makes it easier to cherry pick them if someone needs to cherry-pick a subsequent PR with a downstream migration (ie, we need to cherry pick a PR that has a migration that revises this one). Since this PR only adds a column it can be cherry-picked safely and the logic can be left out.
Additionally, we only merge this PR once the accompanying PR with the logic has also been approved and is ready to merge.
def upgrade(): | ||
with op.batch_alter_table("dbs") as batch_op: | ||
batch_op.add_column( | ||
sa.Column("save_option", sa.VARCHAR(255), server_default="SQL_ALCHEMY") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think save_option
is not a super informative nam), something like configuration_method
would be better. And let's use SQLALCHEMY_URI
as the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good, I appreciate you helping me with naming conventions, it is something that I definitely struggle with.
@AAfghahi it might be better leaving this PR as draft until the one with the logic is implemented and ready to merge. Then you can also link them to each other in the description. |
converted! |
26fd564
to
fec5ef2
Compare
Codecov Report
@@ Coverage Diff @@
## master #14433 +/- ##
==========================================
+ Coverage 77.15% 77.31% +0.16%
==========================================
Files 958 959 +1
Lines 48241 48393 +152
Branches 5636 5636
==========================================
+ Hits 37219 37416 +197
+ Misses 10821 10776 -45
Partials 201 201
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
with op.batch_alter_table("dbs") as batch_op: | ||
batch_op.add_column( | ||
sa.Column( | ||
"configuration_method", sa.VARCHAR(255), server_default="SQLALCHEMY_URI" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this column nullable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is a good question. I think that it should not be nullable, because it is something assigned at creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree.. any existing rows should default to the current method which is the sqlalchemy uri.
5117d8b
to
22508f5
Compare
superset/models/core.py
Outdated
@@ -99,6 +100,11 @@ class CssTemplate(Model, AuditMixinNullable): | |||
css = Column(Text, default="") | |||
|
|||
|
|||
class ConfigurationMethod(str, enum.Enum): | |||
SQLALCHEMY_URI = "sqlalchemy_form" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make the var equal to the value
SQLALCHEMY_URI = "sqlalchemy_form" | |
SQLALCHEMY_FORM= "sqlalchemy_form" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, pushing up now.
29804e7
to
684e9d7
Compare
LGTM, but will let @betodealmeida do the approval |
import sqlalchemy as sa | ||
from alembic import op | ||
|
||
from superset.models.core import ConfigurationMethod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's hard code this to ensure the migration script doesn't get broken if someone changes the enum in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I wasn't sure what method would be preferable, to hard code or to import, looking through the versions I saw a little bit of both.
superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py
Outdated
Show resolved
Hide resolved
939625d
to
277d4f9
Compare
Hi I think this change may have introduced a bug on the front end, i get this error when adding a new database |
hey, I think that issue is set to be fixed via this: #14583 which creates a new database edit. @betodealmeida @eschutho I can make this field optional till then? |
SUMMARY
We are adding a new enum column to the Database model that identifies which method was used in its' creation or updating.
The logic for this is written here: #14451
Reason
This column is necessary so that front end components know what information to present to the user when they are editing a database.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Coming Soon.
ADDITIONAL INFORMATION