Skip to content
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

Allow script-location property to be a list #67

Merged
merged 8 commits into from
Apr 18, 2022

Conversation

rbleuse
Copy link
Contributor

@rbleuse rbleuse commented Mar 30, 2022

Similar to Flyway, we can have the possibility to set multiple location for script paths (https://flywaydb.org/documentation/configuration/parameters/locations)

Job done :

  • set locations to a List
  • keep and deprecate script-location in spring properties for backward compatibility
  • keep scriptPath as a String for backward compatibility in the MigrationRepository class
  • disable JarLocationScannerTest on Windows as it fails (need investigation on that one), cf Failing tests on Windows #57
  • update dependencies (spring-boot, slf4j & cassandra driver)
  • execute tests with embedded-cassandra : no need for a running cassandra instance, this library runs it for us (similar to cassandra-unit)

We can choose which cassandra-unit version to use for tests, just one note is that cassandra itself has drop support for windows since early 4.0 beta, so if we want to enable testing cassandra 4+ on all environment, the only solution will be to use Testcontainers

README.md Outdated Show resolved Hide resolved
@patka
Copy link
Owner

patka commented Apr 12, 2022

Hi @rbleuse, thanks a lot for the PR and sorry for my late response. I will look at it on the weekend. The holidays should finally give me the time to take care of this.

Copy link
Owner

@patka patka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

thanks for doing this change and all the other little improvements that you performed on this. Some of this were on my agenda as well, so thanks again. I just added some minor comments. If you can address them, I will merge it right away.

* @return The locations of the migration scripts. Never null.
*/
public List<String> getScriptLocations() {
return scriptLocations;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to return Collections.unmodifiableList(scriptLocations) to prevent any tempering with the list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for the suggestion

.version("3.11.12")
.build();
cassandra.start();
LOGGER.warn("cassandra started successfully");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these log messages? I assume they were for testing but I don't think that tests need to log messages, especially not at level warning.

.addConfigProperty("enable_user_defined_functions", true)
.build();
cassandra.start();
LOGGER.warn("cassandra started successfully");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

@@ -28,6 +31,11 @@ public class JarLocationScannerTest {
private static File jarFile;
private static URI jarUri;

@Before
public void windowsOnly() {
Assume.assumeFalse(isWindows());
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to provide a message here. Just failing without explanation is kind of rude :)

Copy link
Contributor Author

@rbleuse rbleuse Apr 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. One thing is it doesn't fail, it just doesn't run tests if the Assume is not satisfied.
It's kind of a conditional @Ignore ;)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for explaining :)

@rbleuse
Copy link
Contributor Author

rbleuse commented Apr 17, 2022

I fixed according to your comments. Please check
Thanks !

@rbleuse rbleuse requested a review from patka April 17, 2022 23:31
@patka patka merged commit 182fa13 into patka:master_v4 Apr 18, 2022
@rbleuse rbleuse deleted the multiple_locations branch April 29, 2022 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants