chore(deps): Update dependencies#279
Conversation
reneleonhardt
force-pushed
the
updates
branch
5 times, most recently
from
a2dcb0a to
d86e40e
Compare
reneleonhardt
force-pushed
the
updates
branch
2 times, most recently
from
a2f793e to
820fa28
Compare
tomix26
left a comment
tomix26
left a comment
There was a problem hiding this comment.
I'm not sure if upgrading everything at once is a good idea, but if you manage to do it successfully, then why not.
I've added a few quick comments to your changes to clarify what needs to be addressed before the pull request can be merged.
| "type": "java.lang.String", | ||
| "description": "Docker image containing PostgreSQL database.", | ||
| "defaultValue": "postgres:11-alpine" | ||
| "defaultValue": "postgres:16-alpine" |
There was a problem hiding this comment.
Keep in mind that such changes will have to wait until the next major version.
| "description": "Version of EnterpriseDB PostgreSQL binaries (https://www.enterprisedb.com/download-postgresql-binaries).", | ||
| "defaultValue": "11.10-1" | ||
| "description": "Version of EnterpriseDB PostgreSQL binaries (https://www.enterprisedb.com/download-postgresql-binaries). 11+ for Linux available on https://www.postgresql.org/download/", | ||
| "defaultValue": "10.23-1" |
There was a problem hiding this comment.
The PostgreSQL versions for OpenTable and Yandex providers must match the other providers, regardless of whether they support Linux platform or not. I plan to remove them in the next major version anyway.
| DatabaseProvider databaseProvider = applicationContext.getBean("dockerPostgresDatabaseProvider", DatabaseProvider.class); | ||
|
|
||
| verify(databaseContext, times(4)).reset(); | ||
| verify(databaseContext, atLeast(2)).reset(); |
There was a problem hiding this comment.
This change could significantly affect the library's performance, so it's necessary to carefully consider and justify every such change.
| JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource); | ||
|
|
||
| String collate = jdbcTemplate.queryForObject("show lc_collate", String.class); | ||
| String collate = jdbcTemplate.queryForObject("SELECT datcollate FROM pg_database WHERE datname NOT IN('template0', 'template1', 'postgres') LIMIT 1", String.class); |
There was a problem hiding this comment.
I'd like to keep the test code as simple as possible. So, unless this change is necessary for some reason, I would prefer the original version.
| public void testConfigurationProperties() throws Exception { | ||
| MockEnvironment environment = new MockEnvironment(); | ||
| environment.setProperty("zonky.test.database.postgres.yandex-provider.postgres-version", "9.6.11-1"); | ||
| environment.setProperty("zonky.test.database.postgres.yandex-provider.postgres-version", "10.23-1"); |
There was a problem hiding this comment.
I guess the point of this test is to have a different PostgreSQL version from the default value to verify that changing the property works.
| [name: '7.6.0', flyway: '7.6.0', 'flyway-test': '7.0.0', spring: '5.3.13', 'spring-boot': '2.4.13', 'zonky-postgres': 'default'], | ||
| [name: '7.15.0', flyway: '7.15.0', 'flyway-test': '7.0.0', spring: '5.3.27', 'spring-boot': '2.5.15', 'zonky-postgres': 'default'], | ||
| [name: '8.0.5', flyway: '8.0.5', 'flyway-test': '7.0.0', spring: '5.3.27', 'spring-boot': '2.6.15', 'zonky-postgres': 'default'], | ||
| // [name: '4.0.3', flyway: '4.0.3', 'flyway-test': '4.0.1', spring: '4.3.30.RELEASE', 'spring-boot': '1.5.22.RELEASE', 'zonky-postgres': 'default'], |
There was a problem hiding this comment.
Keep in mind that you can freely add new versions to the version matrix, but all original versions must remain functional. I'm referring to both library/framework versions (Flyway, Liquibase, Spring, Spring Boot, ...) as well as database versions (Postgres, MSSQL, MySQL, MariaDB, ...).
| api 'org.testcontainers:mssqlserver:1.18.3' | ||
| api 'org.testcontainers:mysql:1.18.3' | ||
| api 'org.testcontainers:mariadb:1.18.3' | ||
| api 'org.testcontainers:postgresql:1.20.1' |
There was a problem hiding this comment.
My intention is to support a wide range of versions at runtime, including the latest ones, but when it comes to default versions, these should be held back a bit. For more details on why I believe using the latest versions in projects like this isn't ideal, check out my previous comment:
#260 (comment)
| eachDependency { details -> | ||
| if (details.requested.group == 'junit') { | ||
| details.useVersion "4.12" | ||
| details.useVersion "4.13.2" |
There was a problem hiding this comment.
I just want to warn you in advance that I don't recommend upgrading to JUnit 5 here, because migrating the logic in TestAssumptions isn't trivial. I've tried it at least once before and wasn't successful.
2 participants
No description provided.