Skip to content

feat: Adds CLI commands to execute viz migrations#25304

Merged
michael-s-molina merged 6 commits into
apache:masterfrom
michael-s-molina:cli-viz-migrations
Sep 19, 2023
Merged

feat: Adds CLI commands to execute viz migrations#25304
michael-s-molina merged 6 commits into
apache:masterfrom
michael-s-molina:cli-viz-migrations

Conversation

@michael-s-molina
Copy link
Copy Markdown
Member

@michael-s-molina michael-s-molina commented Sep 14, 2023

SUMMARY

Adds a new CLI command called viz-migrations to allow users to migrate charts of a specific type. This command is particularly helpful to migrate visualizations to the latest version and at the same time disable their legacy versions with the VIZ_TYPE_DENYLIST configuration.

Usage: superset viz-migrations [OPTIONS] COMMAND [ARGS]...

  Migrates a viz from one type to another.

Commands:
  downgrade  Downgrades a viz to the previous version.
  upgrade    Upgrade a viz to the latest version.

TESTING INSTRUCTIONS

1 - Execute an upgrade of a particular viz type.
2 - Downgrade the previously upgraded viz type.
3 - Check the viz versions between these steps.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina michael-s-molina requested a review from a team as a code owner September 14, 2023 17:41
@pull-request-size pull-request-size Bot added size/L and removed size/M labels Sep 15, 2023
@michael-s-molina michael-s-molina marked this pull request as draft September 15, 2023 14:20
@john-bodley john-bodley self-requested a review September 15, 2023 16:48
@michael-s-molina michael-s-molina marked this pull request as ready for review September 15, 2023 17:56
john-bodley
john-bodley reviewed Sep 15, 2023
View reviewed changes
Comment thread superset/cli/viz_migrations.py Outdated


@click.group()
def viz_migrations() -> None:
Copy link
Copy Markdown
Member

@john-bodley john-bodley Sep 15, 2023

Choose a reason for hiding this comment

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

Suggested change
def viz_migrations() -> None:
def viz_migrate() -> None:

Should this be a verb?

Copy link
Copy Markdown
Member Author

@michael-s-molina michael-s-molina Sep 18, 2023

Choose a reason for hiding this comment

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

Yes. I changed it to migrate_viz.

Comment thread superset/cli/viz_migrations.py Outdated
Comment thread superset/migrations/shared/migrate_viz/base.py
bind = op.get_bind()
session = db.Session(bind=bind)
def upgrade(cls, session: Session) -> None:
slices = session.query(Slice).filter(Slice.viz_type == cls.source_viz_type)
Copy link
Copy Markdown
Member

@john-bodley john-bodley Sep 15, 2023

Choose a reason for hiding this comment

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

Can this not be db.session for all occurrences, i.e., CLI and Alembic migration?

Copy link
Copy Markdown
Member Author

@michael-s-molina michael-s-molina Sep 18, 2023

Choose a reason for hiding this comment

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

I tried using db.session but it completely broke CI. I decided to remove session creation from the base class and leave that responsibility with the migration callers. This preserves the current behavior of Alembic migrations and at the same time allows the migration to be called from commands.

Comment thread superset/cli/viz_migrations.py Outdated
migrate_viz(VizTypes(viz_type), is_downgrade=True)


def migrate_viz(viz_type: VizTypes, is_downgrade: bool = False) -> None:
Copy link
Copy Markdown
Member

@john-bodley john-bodley Sep 15, 2023

Choose a reason for hiding this comment

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

Suggested change
def migrate_viz(viz_type: VizTypes, is_downgrade: bool = False) -> None:
def migrate_viz(viz_type: VizTypes, is_upgrade: bool = True) -> None:

I thought upgrading is more typical than downgrading and thus naming the flag is_upgrade seems more appropriate.

Copy link
Copy Markdown
Member Author

@michael-s-molina michael-s-molina Sep 18, 2023

Choose a reason for hiding this comment

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

You're right, upgrading is more typical. The reason I named it is_downgrade is because it's an optional parameter which generally is not set.

john-bodley
john-bodley reviewed Sep 16, 2023
View reviewed changes
Comment thread superset/cli/viz_migrations.py Outdated
@michael-s-molina michael-s-molina mentioned this pull request Sep 18, 2023
Merged
9 tasks
@michael-s-molina michael-s-molina merged commit b722a95 into apache:master Sep 19, 2023
@michael-s-molina michael-s-molina mentioned this pull request Sep 19, 2023
Merged
9 tasks
eschutho pushed a commit to Superset-Community-Partners/superset that referenced this pull request Sep 21, 2023
@michael-s-molina @eschutho
feat: Adds CLI commands to execute viz migrations (apache#25304)
c63c0c7
@michael-s-molina michael-s-molina mentioned this pull request Nov 10, 2023
Merged
9 tasks
@michael-s-molina michael-s-molina mentioned this pull request Nov 20, 2023
Merged
9 tasks
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@michael-s-molina
feat: Adds CLI commands to execute viz migrations (apache#25304)
482df84
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 8, 2024
@mistercrunch mistercrunch added the 🚢 3.1.0 First shipped in 3.1.0 label Mar 8, 2024
@michael-s-molina michael-s-molina mentioned this pull request Mar 28, 2024
Merged
9 tasks
@michael-s-molina michael-s-molina mentioned this pull request May 20, 2024
Merged
9 tasks
@michael-s-molina michael-s-molina mentioned this pull request May 30, 2024
Merged
9 tasks
@michael-s-molina michael-s-molina mentioned this pull request Jun 24, 2024
Merged
9 tasks
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
@michael-s-molina
feat: Adds CLI commands to execute viz migrations (apache#25304)
1928b12
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
@michael-s-molina
feat: Adds CLI commands to execute viz migrations (apache#25304)
e85210d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@john-bodley john-bodley john-bodley left review comments

@justinpark justinpark justinpark approved these changes

@rusackas rusackas Awaiting requested review from rusackas

@kgabryje kgabryje Awaiting requested review from kgabryje

@villebro villebro Awaiting requested review from villebro

Assignees

No one assigned

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.1.0 First shipped in 3.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@michael-s-molina @justinpark @john-bodley @mistercrunch