-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
add preconditions support for include/includeAll tags #6440
base: master
Are you sure you want to change the base?
add preconditions support for include/includeAll tags #6440
Conversation
Hi @cagliostro92, I think if this PR is ready, we can probably work on its review next week. One thing, @wwillard7800 made us notice is that we would need to resolve some merge conflicts once #6381 gets merged since a DatabaseChangeLog refactoring has been made there. I'll make sure to keep you posted about it. At first glance, this PR looks good to me, but we will conduct a deeper review soon. Thanks, |
Hi @MalloD12, |
…Includeall-changes
…Includeall-changes
liquibase-integration-tests/src/test/groovy/liquibase/command/IncludePreconditionsTest.groovy
Outdated
Show resolved
Hide resolved
liquibase-integration-tests/src/test/groovy/liquibase/command/IncludePreconditionsTest.groovy
Outdated
Show resolved
Hide resolved
Hi @cagliostro92, I've been reviewing and doing a bit of manual testing, and this PR looks good to me. I have only posted a few minor comments, but I think this is ready to move forward and allow other team members to look at it. However, if you don't mind, I would like to wait for the approval until making sure the rebase with #6381 is all good. Thanks, |
Hi @MalloD12, |
Refs: #6354
Impact
Description
Fixes #6354
This pull request is intended to add a new feature and allow the end user to leverage preconditions also from inside
<include>
and<includeAll>
tags e.g.To make it possible was added a new
<sequence>
tag for both<include>
and<includeAll>
tags inside dbchangelog-latest.xsd and was changed the DatabaseChangeLog class to allow the inclusion of include and includeAll nodes only after checking the preconditions.The correctness is guaranteed through the following brand new integration tests:
Things to be aware of
@MalloD12 as previously discussed with you be aware that:
In this case not_include files will not be included
While in this case they will be included. This is the reason behind the #1200, and this is our n°2 point discussion in #6354. The global preconditions seem to be always ignored, and changeset/include/includeAll are always parsed and included in the changelog. I'm going to leave this PR in draft state so that you can tell me your preference about the handling of this situation.
Things to worry about
Additional Context