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

Implement mechanism of override and patch migration files #31

Merged
merged 10 commits into from
Jan 22, 2024

Conversation

weronikasosnowskaseqera
Copy link
Contributor

@weronikasosnowskaseqera weronikasosnowskaseqera commented Oct 26, 2023

Description

Closes #30

This PR adds functionality of override and patch files described in this issue https://github.com/seqeralabs/nf-tower-cloud/issues/3613.

The migration tool will scan migration files according to the given pattern like before. Besides collecting migrationEntries, It will also collect files with .patch or .override extensions in the file name and put Them in separate lists. In these collections, we will be looking for override or patch files during the applyMigration method.

Scenarios

  • WHEN file.sql exists and was executed

    • AND file.override.sql exists -> nothing happens
    • AND file.patch.sql exists -> apply patch
  • WHEN file.sql exists and was not executed

    • AND file.override.sql exists -> apply override
    • AND file.patch.sql exists -> apply file.sql and apply patch
    • AND file.patch.sql exists AND file.override.sql exists -> apply override
  • WHEN file.sql doesn't exist but file.patch.sql exists OR file.override.sql exists -> nothing happens

@weronikasosnowskaseqera
Copy link
Contributor Author

I'd like to have more opinions about this behavior mentioned above

WHEN file.sql exists and was not executed AND file.fixed.sql exists AND file.amended.sql exists -> apply amended and apply fix

I'm not sure if in this case we would prefer to execute only amended or both amended and fixed file.

@tcrespog
Copy link
Contributor

I'm not sure if we should couple the amended/fixed logic to the migtool library. Originally, the proposal described at https://github.com/seqeralabs/nf-tower-cloud/issues/3613 suggested general-purpose changes that would allow to implement the logic on the side of the users of the library.

From the point of view of the tool/library, the idea was to:

  1. Add the ability to check if a specific migration file has been applied.
  2. Receive a set of migration files to run.

Therefore, with those capabilites in place, from the point of view of users of the tool, the amended/fixed logic could be applied by:

  1. Getting all files to run and identifying those "broken" files with amended and fix companions.
  2. Checking whether the broken migrations and companions were already applied with migtool.
  3. Choose the .fix or .amended files depending on whether the broken file was already applied or not.
  4. Send the specific subset of files to execute.

I'm not sure if in this case we would prefer to execute only amended or both amended and fixed file.

The idea in this case is to ignore the original (and .amended) and apply .fixed. In a nutshell:

  • Original already applied: run .amended.
  • Original not applied: run .fixed.

@weronikasosnowskaseqera
Copy link
Contributor Author

After talking with @tcrespog decided to close this PR and prepare another one with less changes. We will handle fix and amend files on the platform side.

@pditommaso
Copy link
Contributor

We will handle fix and amend files on the platform side.

Sorry to be late on this, but not sure that a good idea. This tools is used across different projects, tower, licman, groundswell thefore it has more sense to have this logic as a core feature

@weronikasosnowskaseqera
Copy link
Contributor Author

@pditommaso okay, then we can reopen this PR. I tested It with the platform by running migration with all migration files and I added a few fixed, and amended files to test new functionality - all worked well.

When we have this PR merged I will open another PR with more e2e tests on the platform side and actual fixed and amended files described in this issue.

For visibility: I created similar PR for the platform, logic is the same. If we decided that we will place these changes here, in this repo - I will close that PR.

@weronikasosnowskaseqera weronikasosnowskaseqera added the enhancement New feature or request label Nov 24, 2023
@weronikasosnowskaseqera weronikasosnowskaseqera marked this pull request as ready for review November 24, 2023 13:28
Copy link
Contributor

@tcrespog tcrespog left a comment

Choose a reason for hiding this comment

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

Worth adding adding a description about the .fixed/.amended files support in the README file.

Copy link
Contributor

@tcrespog tcrespog left a comment

Choose a reason for hiding this comment

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

I've just noticed a few things that I overlooked initially in the tests, but the rest is fine.

@swampie swampie force-pushed the 30-implement-amended-and-fixed-files branch 3 times, most recently from 64decfd to b4e3888 Compare January 8, 2024 11:34
@weronikasosnowskaseqera weronikasosnowskaseqera force-pushed the 30-implement-amended-and-fixed-files branch from aa32016 to b8af571 Compare January 9, 2024 08:35
@weronikasosnowskaseqera
Copy link
Contributor Author

@pditommaso Can you review this one, please?

@pditommaso
Copy link
Contributor

Thanks. I'll check asap

@weronikasosnowskaseqera weronikasosnowskaseqera changed the title Implement mechanism of amended and fixed migration files Implement mechanism of override and patch migration files Jan 18, 2024
@swampie
Copy link
Member

swampie commented Jan 19, 2024

@pditommaso can you please merge and start a release?

@weronikasosnowskaseqera
Copy link
Contributor Author

I want to run It again with tower after the file pattern changes just in case. I will put a comment here when ready.

@pditommaso
Copy link
Contributor

can you please merge and start a release?

we need fix before the current problem, I don't want to overlap things

@weronikasosnowskaseqera
Copy link
Contributor Author

All right, I managed to run It locally with ./gradlew publishToMavenLocal on the branch with patch and override files https://github.com/seqeralabs/nf-tower-cloud/pull/6160. We need to release this PR to continue with the mentioned PR.

@pditommaso
Copy link
Contributor

Ok

@pditommaso pditommaso merged commit cbefab5 into master Jan 22, 2024
@pditommaso pditommaso deleted the 30-implement-amended-and-fixed-files branch January 22, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement amended and fixed files
4 participants