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

Fix migration script for v1.2 upgrade #651

Merged
merged 3 commits into from
Aug 14, 2023

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Aug 10, 2023

Feature or Bugfix

  • Bugfix

Detail

  • migration script for upgrade to V1.2 had a mistake and is affecting one customer. Basically the devStrategy and devStages values were not backfilled which causes nulls in the RDS table that are not allowed as this column should contain only non-null values.

In this PR we modify that script for customers that have not updated yet. It is not 100% clear to me whether we should merge it, but I wanted to raise awareness of this issue here.

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dlpzx dlpzx requested a review from nikpodsh August 11, 2023 07:35
Copy link
Contributor

@louishourcade louishourcade left a comment

Choose a reason for hiding this comment

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

Approved.
Thanks @dlpzx, I let you do the merge


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
# Modify column types
print("Upgrade devStages and devStrategy column types. Updating nullable to True...")
op.add_column(
'datapipeline',
sa.Column('template', sa.String(), nullable=True)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we declare nullable=True for devStrategy and devStages if both columns are originally nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The important part of this stage is to update the existing_type

@dlpzx dlpzx merged commit c189de4 into main Aug 14, 2023
nikpodsh added a commit that referenced this pull request Aug 16, 2023
Merge latest changes from main into modularization-main

It includes changes from #626, #630, #648, #649, and #651

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dlpzx <71252798+dlpzx@users.noreply.github.com>
Co-authored-by: wolanlu <101870655+wolanlu@users.noreply.github.com>
Co-authored-by: Amr Saber <amr.m.saber.mail@gmail.com>
Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: kukushking <kukushkin.anton@gmail.com>
Co-authored-by: Dariusz Osiennik <osiend@amazon.com>
Co-authored-by: Dennis Goldner <107395339+degoldner@users.noreply.github.com>
Co-authored-by: Abdulrahman Kaitoua <abdulrahman.kaitoua@polimi.it>
Co-authored-by: akaitoua-sa <126820454+akaitoua-sa@users.noreply.github.com>
Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com>
Co-authored-by: Rick Bernotas <97474536+rbernotas@users.noreply.github.com>
Co-authored-by: David Mutune Kimengu <57294718+kimengu-david@users.noreply.github.com>
Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com>
Co-authored-by: Dhruba <117375130+marjet26@users.noreply.github.com>
Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com>
Co-authored-by: Srinivas Reddy <srinivasreddych@outlook.com>
Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
Co-authored-by: Noah Paige <noahpaig@amazon.com>
Co-authored-by: dlpzx <dlpzx@amazon.com>
@dlpzx dlpzx deleted the bugfix/migration-scripts-sqlpipelines branch September 5, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants