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

Recalculate checksums #2

Merged
merged 4 commits into from
Feb 9, 2024
Merged

Recalculate checksums #2

merged 4 commits into from
Feb 9, 2024

Conversation

riiwo
Copy link

@riiwo riiwo commented Feb 2, 2024

We have diverged from the original repo and it does not make sense anymore for us to try keeping up to date with the original.
Why? Because we rely on undo functionality that was not accepted to the schemachange: Snowflake-Labs#158

Taking inspiration from Flyway, this commits adds the undo subcommand to
the application, with the `--step <n>` flag.

Undo scripts will have the `U` prefix, with the same name and version as
their corresponding versioned script. It will attempt to undo <n> amount
of versioned scripts until it finishes, or no corresponding undo script
is found.

Under the hood, the application is removing the `V` script from the
change history table, so when the `deploy` command is ran, the versioned
script can be applied again.

Algorithm:

* Fetch all applied `V` migrations
* Fetch all `U` migrations
* Get `U` migration of last `V` migration.
* IF `V` migration has no `U` migration, process stops
* Apply `U` script
* Delete `V` script from the change_history_table related to `U`

This commit closes issue Snowflake-Labs#19
@riiwo riiwo force-pushed the recalculate-checksums branch 4 times, most recently from da59b37 to a7ee0d0 Compare February 5, 2024 07:21
@riiwo riiwo force-pushed the recalculate-checksums branch 6 times, most recently from 90395c8 to 5c4b02c Compare February 6, 2024 08:09
[Swithing to Debian 12](salemove/base-images#46) in python base image
caused [OpenSSL switch](salemove/base-images#46 (comment)) from 1.1.1 to 3.0.

Current version of `snowflake-connector-python` 2.8.0 which depends on
`pyopenssl` 22.1.0 fails because of OpenSSL switch https://ci.at.samo.io/job/salemove/job/snowflake-ingestion/job/PR-589/8/consoleFull

After the update of `snowflake-connector-python` to 3.7.0 `pyopenssl`
becomes `23.3.0` and doesn't fail anymore in this image.

Tested `snowflake-ingestion` in [this PR](salemove/snowflake-ingestion#600).
[Pipeline](https://ci.at.samo.io/job/salemove/job/snowflake-ingestion/job/PR-600/6/consoleFull)
doesn't fail anymore, and does migration/rollback successfully.

CBRO-1908
@riiwo riiwo force-pushed the recalculate-checksums branch 2 times, most recently from ec0d14e to 62264a1 Compare February 8, 2024 20:59
Copy link

@alcidesteixeira alcidesteixeira left a comment

Choose a reason for hiding this comment

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

Yeah, I'll approve.
We can always go back to the main project if it changes some time.
But from the look of the approvers comments, that won't happen anytime soon.

Their alternative doesn't work with us - the always migrate up path.

@@ -31,7 +35,7 @@ All notable changes to this project will be documented in this file.
- Cleaned up argument passing and other repetitive code using dictionary and set comparisons for easy maintenance. (Converted variable names to a consistent snake_case from a mix of kebab-case and snake_case)
- Fixed change history table processing to allow mixed case names when '"' are used in the name.
- Moved most error, log and warning messages and query strings to global or class variables.
- Updated readme to cover new authentication methods
- Updated readme to cover new authentication methods
Copy link

@BondarenkoStas BondarenkoStas Feb 9, 2024

Choose a reason for hiding this comment

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

Please rebase from CBRO-1218-undo-migrations and merge this PR into it. To later merge that branch into master.
The top commit from the branch already removes trailing whitespaces and tabs 94deeb5.

It will be easier to review this PR if it has your changes only.
Also feels unnecessary to write a bunch of logic in recalculate_checksum_command only to split it in the next commit, why not to split it initially?

Copy link
Author

@riiwo riiwo Feb 9, 2024

Choose a reason for hiding this comment

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

why not to split it initially

One was feature, other was refactoring

Copy link
Author

Choose a reason for hiding this comment

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

anyways, it is rebased. I'll still point to master. You can skip first 3 commits

Choose a reason for hiding this comment

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

thank you

setup.cfg Outdated Show resolved Hide resolved
@riiwo riiwo force-pushed the recalculate-checksums branch 2 times, most recently from ed73f27 to ce5a9c2 Compare February 9, 2024 12:09
In case you clone a database, you would want to recaclulate checksums for
repeatable scripts as they already exist but checksum is wrong as database name
has changed.

Both deploy_command and recalculate_checksum use very similar body. Only
differences are:

1) Deploy deals with V, R, A migrations, while recalculation deals only with
R migrations.
2) Deploy also applies the script, while recalculation on updates
schemachangetables.
Copy link

@BondarenkoStas BondarenkoStas left a comment

Choose a reason for hiding this comment

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

Very nice, especially apply parameter in apply_scripts :)

@riiwo riiwo merged commit b700f71 into master Feb 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants