Closed alexantonica closed 3 years ago
Hi @alexantonica
thanks for the pull request. I see that I will have a look at it on the weekend.
Have a nice day Patrick
I would like to ask you to explain the scenario that you really have here. I am having a little bit of trouble with this feature because the whole idea of script based migrations is that you can say for sure that a database at version x has a certain state and that this is reproducible. If you go ahead like this it could mean that one developer bumps his database to version 5 with one failing migration in between and another developer is unable to do this because he does not have the failGracefully property set. Besides that, what is the strategy to move forward then? I mean, the failing script has to be fixed and then copied to a new script that is fixed (since the database believes it successfully executed all scripts), so I wonder what the benefit is over fixing it immediately and rerun the process. Furthermore one failing script can easily result in consecutive failures as later scripts might depend on something that was supposed to be done before. Overall this does not sound like a good idea to me.
So that scenario was that we wanted to have a way to control the migration in case one of the scripts fail. For example if we have 2 alter commands on the same table with the same column name, the script migration would fail. Having the same script can be due to wrong rebasing actions for example and for now there is no way to bypass this. We would have to take all the scripts and put them in a proper order with without replicating the contents. Sometimes this issue is not a good enough reason to manually fix the scripts.
@patka any news on this pls? thanks :)
Hi @alexantonica, sorry for the delay, I was on vacation.
I get your reasoning and I had this kind of issue myself already but I'm afraid this is not a good enough reason for me. To me this is a very dangerous approach and IMHO it is only a question of when and not if this will fall on your feed. If you train your developers to ignore erros during migrations because in 90% of the cases they are false positives, you will inevitably end up with an issue in production that everybody ignored on the stages before.
However, since I remember there was once a similar request to ignore certain exceptions and in order to create more room to extend the functionality, I am currently working on an event mechanism that will allow you to define custom handlers. The events that will be fired will be beforeScriptExecution, afterScriptExection and afterScriptFailure. This will allow you to handle the failure yourself in a way that is not the current default implementation. This will give you the opportunity to implement the graceful failure without me providing the gun pointing on the developers feet to everybody :-)
Would this be ok with you? I would merge it in a branch first and will let you know. So you can have it look if it would solve your problem.
Hi @alexantonica, sorry for the delay, I was on vacation.
I get your reasoning and I had this kind of issue myself already but I'm afraid this is not a good enough reason for me. To me this is a very dangerous approach and IMHO it is only a question of when and not if this will fall on your feed. If you train your developers to ignore erros during migrations because in 90% of the cases they are false positives, you will inevitably end up with an issue in production that everybody ignored on the stages before.
However, since I remember there was once a similar request to ignore certain exceptions and in order to create more room to extend the functionality, I am currently working on an event mechanism that will allow you to define custom handlers. The events that will be fired will be beforeScriptExecution, afterScriptExection and afterScriptFailure. This will allow you to handle the failure yourself in a way that is not the current default implementation. This will give you the opportunity to implement the graceful failure without me providing the gun pointing on the developers feet to everybody :-)
Would this be ok with you? I would merge it in a branch first and will let you know. So you can have it look if it would solve your problem.
Sounds good @patka . Thank you 👍 . Gonna close this PR then.
What does this PR do?
This PR:
shouldFailGracefully
toMigrationTask
in order to better control what happens when, in a batch script migration, of the scripts failDbMigration
public class in order for it to be extendeddatabaseIsUpToDate
protected in order for it to be overriddenMigrationTask
and setter forshouldFailGracefully