-
Notifications
You must be signed in to change notification settings - Fork 32
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
enabling loading of extenssions #36
Conversation
Thank you for the contribution. I'll take a look at it as soon as I get the chance. I am curious about one thing: What does the maven-publish plugin do that we can't already do with |
I couldn't figure out how to publish to my local maven. Should I remove? |
I had some time to take a deeper look at the code. I really like the idea of getting changes from the registry. In fact, I think all changes should be fetched from the registry. This could be done easily enough by putting the registry lookup in its own helper and having all methods, including First, how would we process external changes that have nested elements? The DSL represents nested elements as closures, but I can't think of any way to up a delegate for the closures in external changes. For now this could be a documented limitation since it lets us process more than we used to be able to handle. Second, the new DSL code will ignore certain invalid change set code. For example, There are a couple of other bugs in the DSL I need to look into, but then I can look into this further. I think we can make this work. |
Hi,
Thanks for the review. I have thought about the first challenge. I can
think of a messy way but it's a long shot.
As for the second problem, I believe the validation should be left for the
`Change` implementations. I just skimmed through the code and I think we
can reject the changeset
in `org.liquibase.groovy.delegate.ChangeSetDelegate#makeColumnarChangeFromMap`
if
it has a closure and doesn't have either a where clause or some columns.
IMHO I don't think to let users define a closure that does nothing is not a
bad thing.
Thanks for the review.
…On Sat, Oct 6, 2018 at 10:12 PM Steven C. Saliman ***@***.***> wrote:
I had some time to take a deeper look at the code.
I really like the idea of getting changes from the registry. In fact, I
think all changes should be fetched from the registry. This could be done
easily enough by putting the registry lookup in its own helper and having
all methods, including methodMissing use that helper. This would work for
most use cases, but it does leave a couple of challenges...
First, how would we process external changes that have nested elements?
The DSL represents nested elements as closures, but I can't think of any
way to up a delegate for the closures in external changes. For now this
could be a documented limitation since it lets us process more than we used
to be able to handle.
Second, the new DSL code will ignore certain invalid change set code. For
example,
addAutoIncrement(tableName: 'myTable') { def x = 4 } currently complains
because a closure is not valid for the addAutoIncrement change, but the
new code would not complain. I don't like this because the code should not
report success if it doesn't do everything it was asked to do. I think I
could work around that problem by having a list of changes that support
closures and throwing an exception if a given change has a closure and
isn't on the list.
There are a couple of other bugs in the DSL I need to look into, but then
I can look into this further. I think we can make this work.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG_trmxup7hQvrSV5rInFQWU2o6poWnPks5uiQC2gaJpZM4WoTWz>
.
|
Here is what I think I want to do:
IMHO, I think when users put something in a changelog they intend for Liquibase to do something with it, and Liquibase should not report success if it didn't run everything the user intended. I should have some time over the weekend to work on this. |
I've implemented this fix, and resolved a few other issues at the same time:
The changes are pushed, but I want to look into a possible Spring Boot bug before I make a release. |
Hi Steven
I'm sorry I couldn't help last weekend. I'm using it with spring boot, I've
not had a problem. But let me know if you need help.
Amanuel Nega (Software Engineer)
+251 947 01 25 21
…On Sun, Oct 14, 2018 at 9:45 PM Steven C. Saliman ***@***.***> wrote:
I've implemented this fix, and resolved a few other issues at the same
time:
1.
I implemented a lookupChange method as discussed before.
2.
I changed makeColumnarChange and makeChangeFromMap to use the new
method.
3.
I implemented a methodMissing method. It will only handle changes with
no arguments, or a single map argument. Anything else will throw an
exception to let users know that they've asked us to process a change we
can't parse. I also fixed a bug that was causing errors in the way changes
were added to a rollback.
4.
I eliminated all of the standard changes that take a single map and
had no special processing. They can flow through methodMissing now.
5.
I fixed the test that was failing because of the unexpected debug
output from the Liquibase registry lookup - I did it by adding a
logback-test.xml file to send the output to stderr instead of stdout.
6.
I moved the sample extension from src/main to src/test so we weren't
shipping it with the DSL, but we could still use it to test the processing
of extension based changes.
The changes are pushed, but I want to look into a possible Spring Boot bug
before I make a release.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG_trsMj9IjBaGA3DmDixykHitzD0bMNks5uk4ZdgaJpZM4WoTWz>
.
|
I just released version 2.0.2 of the Groovy DSL with the changes that allow extension provided changes to be loaded. Thank you for all of your help on this issue. |
Unless there is a complex logic we need to do to parse the groovy change, we can look up the liquibases registry to find the appropriate change.
What I did was to look up the change in the methodMissing callback as that is the perfect place to look for it.
Also minor changes to the build file to enable publishing to mavenLocal