-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Change to receive files outside the classpath in mongodb liquibase #39680
Conversation
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.
Thanks a lot for this pull request! I like what you did here and bonus point for making sure we weren't breaking the previous behavior.
Any chance you could have a look if the standard Liquibase could use the same treatment (it could be done in another PR)?
Also, I added a few very small comments.
...base-mongodb/runtime/src/main/java/io/quarkus/liquibase/mongodb/LiquibaseMongodbFactory.java
Outdated
Show resolved
Hide resolved
...base-mongodb/runtime/src/main/java/io/quarkus/liquibase/mongodb/LiquibaseMongodbFactory.java
Outdated
Show resolved
Hide resolved
I just uploaded a commit with the changes you suggested. Now about checking the standard Liquibase, sure! no problem, as soon as I have some free time I will check the code to make sure that the same treatment is done to the changelog. |
Already did the PR in the standard Liquibase extension. |
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.
LGTM, thanks.
As I saw that @gsmet already look at it I'll let him merge.
This comment has been minimized.
This comment has been minimized.
Thanks for this! I would like to have (at least) the last two commits squashed into the previous ones - FWIW if I made this change I would probably just make it one commit |
Ready the squash of the last two commits into a single one. |
I looked at the commits again and actually I think it makes a lot more sense to have a single one since are all about one new feature that is continuously revised. Those revisions make sense when developing, but they don't add much once the feature is complete. Thereofore. can you please squash into a single commit? Once that's done we can merge (assuming CI is all green) |
Ready the squash of all commits into a single one. |
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.
Thanks!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Changing contais to startsWith Implementing CompositeResourceAccessor and the search-path property Setting default value of the search path and an error in Paths.get() Improving validation with search path default value making suggested improvements formatting code sorting imports Cleaning code
Status for workflow
|
I'll have a look next week. Thanks for your contributions! |
Due to the nature in which the resourceAccessor was created and injected when instantiating the Liquibase class
in LiquibaseMongodbFactory.createLiquibase()
try (ClassLoaderResourceAccessor resourceAccessor = new ClassLoaderResourceAccessor(Thread.currentThread().getContextClassLoader()))
it was not possible to pass a changeLog outside the classpath via
liquibase-mongodb.change-log
property. For people like me who want to use the quarkus-liquibase-mongodb extension only with the dev and test profiles in conjunction with the devservices and in productive environments use the liquibase cli directly this implementation is a bit problematic.I also took the opportunity to implement the search-path property described in: docs.liquibase which allows me to limit the scope of the DirectoryResourceAccessor and not give it the default value of
DirectoryResourceAccessor(Paths.get("/"))
.