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 issues and tighten the CI upgrade/downgrade test #25869

Merged
merged 4 commits into from
Aug 23, 2022

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented Aug 22, 2022

There was a typo that was not detected in the migration file which prevented upgrades.
On fixing the typo, some other migration issues were detected.
Apart from postgres, other dbs couldn't upgrade -> downgrade -> upgrade. This was partly
because we now create new DB from the ORM which also uses a naming convention to create constraints.

I have applied a fix, the side effect is that it breaks offline generation of sql for mysql. This is because it uses the new naming convention to create constraints when we run upgrade through the migration file. In
migration file 0093, we dropped unique keys with names dag_id & dag_id_2 while with the naming
convention in place, these two now have a unique name across all db that's different from the names
we use to drop the keys. Because of that, I have chosen to use sqlalchemy inspector to get the name
and drop them.

@potiuk
Copy link
Member

potiuk commented Aug 22, 2022

I think those updated tests are detecting one more problem with downgrade :)

@ephraimbuddy
Copy link
Contributor Author

I think those updated tests are detecting one more problem with downgrade :)

Yes. A couple more problem. I'm attempting fixes now and tightening the tests further

@ephraimbuddy ephraimbuddy force-pushed the fix-typo-migration branch 3 times, most recently from b1ca1eb to 2cacff9 Compare August 22, 2022 18:10
@ephraimbuddy ephraimbuddy changed the title Fix typo that crashes migration upgrade Fix migration issues and tighten the CI upgrade/downgrade test Aug 22, 2022
@ephraimbuddy
Copy link
Contributor Author

It looks like the failing tests are not caused by the PR. Have rerun it before and rebased but it keeps failing cc: @potiuk

@potiuk
Copy link
Member

potiuk commented Aug 22, 2022

It looks like the failing tests are not caused by the PR. Have rerun it before and rebased but it keeps failing cc: @potiuk

The PRs that are changing "build" scripts running in "upgrade to newer deps" mode. The "warnings" issues (caused by new urrllib3 should be handled by #25885 (please approve). I am not sure what PRod image failure is about (lookign into it).

@potiuk
Copy link
Member

potiuk commented Aug 22, 2022

Possible fix to Google failing prod image tests: #25886

(BTW. It's great we have the tests - they are finding some unexpected problems our users would have only see after they got the release in their hands - seems that there is an implicit dependency in google provider's dependencies on protobuf version)

@potiuk
Copy link
Member

potiuk commented Aug 22, 2022

Should be fixed now. please rebase @ephraimbuddy :)

There was a typo that was not detected in the migration file which prevented upgrades.
On fixing the typo, some other migration issues were detected.
Apart from postgres, other dbs couldn't upgrade -> downgrade -> upgrade. This was partly
because we now create new DB from the ORM which also uses a naming convention to create constraints.

I have applied a fix, the side effect is that it breaks offline generation of sql for mysql. This is because it uses the new naming convention to create constraints when we run upgrade through the migration file. In
migration file 0093, we dropped unique keys with names dag_id & dag_id_2 while with the naming
convention in place, these two now have a unique name across all db that's different from the names
we use to drop the keys. Because of that, I have chosen to use sqlalchemy inspector to get the name
and drop them.
@ephraimbuddy
Copy link
Contributor Author

Should be fixed now. please rebase @ephraimbuddy :)

Thanks

@ephraimbuddy ephraimbuddy merged commit 87c7867 into apache:main Aug 23, 2022
@ephraimbuddy ephraimbuddy deleted the fix-typo-migration branch August 23, 2022 06:51
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dev-tools type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants