Skip to content

test: Clean up MySQL tests#13565

Merged
kodiakhq[bot] merged 2 commits intocloudquery:mainfrom
erezrokah:test/cleanup_mysql
Sep 1, 2023
Merged

test: Clean up MySQL tests#13565
kodiakhq[bot] merged 2 commits intocloudquery:mainfrom
erezrokah:test/cleanup_mysql

Conversation

@erezrokah
Copy link
Copy Markdown
Member

Summary

No need for the loop in the tests or the the sleep. Also we can support lists and maps if we increase innodb_log_file_size.

Copy link
Copy Markdown
Contributor

@mnorbury mnorbury left a comment

Choose a reason for hiding this comment

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

LGTM

@erezrokah erezrokah added the automerge Automatically merge once required checks pass label Sep 1, 2023
Comment on lines +37 to +41
AddColumn: true,
AddColumnNotNull: false,
RemoveColumn: true,
RemoveColumnNotNull: false,
ChangeColumn: false,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
AddColumn: true,
AddColumnNotNull: false,
RemoveColumn: true,
RemoveColumnNotNull: false,
ChangeColumn: false,
AddColumn: true,
RemoveColumn: true,

@kodiakhq kodiakhq bot merged commit 6d41546 into cloudquery:main Sep 1, 2023
kodiakhq bot pushed a commit that referenced this pull request Sep 1, 2023

#### Summary

Saw this while working on #13486. If you run migration twice on the same spec it fails. The reason is that MySQL Implicitly sets PKs to non null in the DB, so we have to account for that.

We had this in the code and it seems it got lost in the migration to v3 or v4.
~~I also cleaned up the tests as those are passing without that loop on test options.~~ Tests cleanup moved to #13565

Not sure why the double migration tests didn't catch this

<!--
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants