-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[DAT-16377] Verify if supports method is implemented in a change to prevent unexpected behaviors #5903
Conversation
…it's not part of the default liquibase changes
//we only verify supports method if this is not part of the default liquibase changes | ||
for (Change plugin : plugins) { | ||
String packageName = plugin.getClass().getPackage().getName(); | ||
if (packageName.startsWith("liquibase.change") || packageName.startsWith("com.datical.liquibase.ext")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I keep pro package here or should we verify it too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is okay to keep it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I think you should remove it since it's an extension.
This reverts commit 3b47101.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for this?
liquibase-cli/src/test/groovy/liquibase/integration/commandline/LiquibaseCommandLineTest.groovy
Outdated
Show resolved
Hide resolved
//we only verify supports method if this is not part of the default liquibase changes | ||
for (Change plugin : plugins) { | ||
String packageName = plugin.getClass().getPackage().getName(); | ||
if (packageName.startsWith("liquibase.change") || packageName.startsWith("com.datical.liquibase.ext")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact I think you should remove it since it's an extension.
Controls the level of validation performed on the | ||
supports method of Change classes. Options are | ||
OFF, WARN, FAIL. | ||
DEFAULT: WARN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the default be FAIL so that we notice if we violate this rule when we run tests in all of the extensions? If we just warn, we probably won't ever actually notice until runtime, at which point it will be too late.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change to FAIL, but this could also break existing extensions. I think we should start with WARN for 1-2 releases and then switch to FAIL - I can create a dat ticket for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do and put it in a future sprint.
This reverts commit 5b89de9.
if (packageName.startsWith("liquibase.change")) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even want this? Thinking more about this, don't we want all changes to be verified? If so, then I think this method just introduces another way for an extension to accidentally override the supports method and us not notice.
liquibase-standard/src/main/java/liquibase/change/ChangeFactory.java
Outdated
Show resolved
Hide resolved
…y.java typo Co-authored-by: Steven Massaro <steven.massaro.web@gmail.com>
Impact
Description
Things to be aware of