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

New config option to disable validateMigrationsList() #3448

Merged
merged 2 commits into from Oct 12, 2019
Merged

New config option to disable validateMigrationsList() #3448

merged 2 commits into from Oct 12, 2019

Conversation

arrilot
Copy link
Contributor

@arrilot arrilot commented Sep 20, 2019

@elhigu here #1747 (comment)

So if you make feature request for some option to disable migration directory consistency check I don't see why it couldn't be implemented if someone is motivated enough to do it.

So here we go

Motivation:
We use gitflow with features in different branches and deploy them separately to stage environment.
All branches on stage environment have one common database.
As a result we constantly face the same issue:

  1. Developer A makes a new migration A in a new branch A. Branch A is deployed on stage
  2. Developer B makes a new migration B in a new branch B (has nothing to do with branch A). Branch B cannot be deployed on stage because "The migration directory is corrupt".

Rolling back migrations is not a solution because not every migration can be rolled back
Reseting database is not a solution because it erases data in database aswell
Merging branch A into B is not a solution because it breaks independency of features.

We know that switching off migration order brings some inconsistency issues but they are far less important for our local/stage environments.

Some other migrators like node-pg-migrate have such feature. (https://github.com/salsita/node-pg-migrate/blob/master/docs/cli.md check-order)

@arrilot arrilot changed the title WIP config option to disable validateMigrationsList New config option to disable validateMigrationsList() Sep 20, 2019
@elhigu
Copy link
Member

elhigu commented Sep 22, 2019

Rolling back migrations is not a solution because not every migration can be rolled back
Reseting database is not a solution because it erases data in database aswell
Merging branch A into B is not a solution because it breaks independency of features.

This is exactly the reason why I have been slightly against of having this feature included to knex. I haven't found many cases where having the switch actually would make sense and would not cause user's system to be inconsistent and broken.

I know this is a bit harsh, but to be honest your process of using staging environment is horribly wrong. It is inviting strange errors, which will appear only in staging environment and it doesn't make sure that multiple merged features actually works correctly when combined.

Effectively staging environment should mirror final production environment to verify that everything is fine before putting it to production.

If you need to test out every branch first separately in staging environment with shared database you should also clean up all changes done to DB when you revert the certain features code form the environment.

Reseting database is not a solution because it erases data in database aswell

Reseting database is the solution, you just need to take SQL dump of the database from the stage where it was before starting to test multiple feature branches. Then before or after running tests for each branch just restore database state from that SQL dump.

That way you will always have consistent database state with code. If you just run bunch of migrations from various features can make completely working feature to seem like broken one (just to name one: migrations also can remove data that other branch might need).

Anyhow IMO that kind of testing should be done in testing environment instead of staging. In staging you should apply each feature on top of each other the same way it will be done finally to the production system.

@arrilot
Copy link
Contributor Author

arrilot commented Sep 30, 2019

Effectively staging environment should mirror final production environment to verify that everything is fine before putting it to production.
Anyhow IMO that kind of testing should be done in testing environment instead of staging. In staging you should apply each feature on top of each other the same way it will be done finally to the production system.

Looks like in my description i've used a term "stage environment" in a misleading way.
I've meant test environment, not preproduction environment. Sorry about that.

A little bit more info about our setup:
Environments: local (developers' PCs) -> test -> preproduction -> production. Each of them has their own database
In preproduction envorinment everything is fine since only migrations from release branch are applyied there.
All of the discussion is about test environment where develop branch and many feature/x branches co-exist linked to a shared DB
In this environment QAs test features before merging them to develop branch. They also test develop branch before releasing it to preproduction/production.

I agree that sharing a database is kinda asking for troubles and not ideal, but we can't spin up a new database for every feature branch due to infrastracture limitations. We can probably make a separate database for develop in the future, though.

Reseting database is the solution, you just need to take SQL dump of the database from the stage where it was before starting to test multiple feature branches. Then before or after running tests for each branch just restore database state from that SQL dump.

Reseting brings new problems to the table:

  1. DevOps need to set up this resetting workflow in Jenkins (we have dozens of services btw)
  2. It will be annoyning to constantly reset database every time a QA wants to test a feature. What if two QAs want to test different features at the same time? They are going to block and interfere with each other.

If you just run bunch of migrations from various features can make completely working feature to seem like broken one (just to name one: migrations also can remove data that other branch might need).

We try to make all the features (migrations) backward compatible as much as possible. It also helps with AB tests and rolling back releases from production in case of accident.

As a result we prefer approach from this PR.

@elhigu
Copy link
Member

elhigu commented Sep 30, 2019

A little bit more info about our setup:
Environments: local (developers' PCs) -> test -> preproduction -> production. Each of them has their own database
In preproduction envorinment everything is fine since only migrations from release branch are applyied there.
All of the discussion is about test environment where develop branch and many feature/x branches co-exist linked to a shared DB
In this environment QAs test features before merging them to develop branch. They also test develop branch before releasing it to preproduction/production.

I agree that sharing a database is kinda asking for troubles and not ideal, but we can't spin up a new database for every feature branch due to infrastracture limitations. We can probably make a separate database for develop in the future, though.

Reseting database is the solution, you just need to take SQL dump of the database from the stage where it was before starting to test multiple feature branches. Then before or after running tests for each branch just restore database state from that SQL dump.

Reseting brings new problems to the table:

  1. DevOps need to set up this resetting workflow in Jenkins (we have dozens of services btw)

It could be integrated as part of test command and all services could be stored / restored by the same script.

  1. It will be annoyning to constantly reset database every time a QA wants to test a feature. What if two QAs want to test different features at the same time? They are going to block and interfere with each other.

Ok, so this is environment where you are hand testing stuff... so there is no tests script where one could do the save/restore etc. Well sounds like you are out of luck then...

If you just run bunch of migrations from various features can make completely working feature to seem like broken one (just to name one: migrations also can remove data that other branch might need).

We try to make all the features (migrations) backward compatible as much as possible. It also helps with AB tests and rolling back releases from production in case of accident.

As a result we prefer approach from this PR.

Good luck!

Copy link
Member

@elhigu elhigu left a comment

Choose a reason for hiding this comment

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

PR is missing tests and documentation

@arrilot arrilot changed the title New config option to disable validateMigrationsList() WIP New config option to disable validateMigrationsList() Oct 5, 2019
@arrilot
Copy link
Contributor Author

arrilot commented Oct 5, 2019

PR is missing tests and documentation

@elhigu done

Looks like build is failing due to totally unrelated thing

  -    "balance": 12.240001
  +    "balance": 12.24

@arrilot arrilot changed the title WIP New config option to disable validateMigrationsList() New config option to disable validateMigrationsList() Oct 5, 2019
@kibertoad
Copy link
Collaborator

@arrilot Pull from master, that test should be fixed now (there is a failing CLI test now, feel free to ignore it)

@arrilot
Copy link
Contributor Author

arrilot commented Oct 7, 2019

@arrilot Pull from master, that test should be fixed now (there is a failing CLI test now, feel free to ignore it)

Done, thanks

@arrilot arrilot requested a review from elhigu October 11, 2019 11:40
@kibertoad kibertoad merged commit 6b9fb0e into knex:master Oct 12, 2019
@arrilot arrilot deleted the disable_migrations_list_validation_feature branch November 12, 2019 09:10
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.

None yet

3 participants