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

Update Scala, sbt and dependencies #46

Closed
wants to merge 5 commits into from

Conversation

guizmaii
Copy link
Contributor

@guizmaii guizmaii commented Oct 8, 2024

Fixes #45

@AugustNagro There's no CI configured? 🤔
Do you want me to configure a minimal Github Actions CI?

@guizmaii guizmaii force-pushed the updates branch 2 times, most recently from 9e51f7c to 27ca74a Compare October 8, 2024 23:52
@guizmaii guizmaii changed the title Update sbt and dependencies Update Scala, sbt and dependencies Oct 8, 2024
Update dependencies

Update Scala

Update munit code
Copy link
Owner

@AugustNagro AugustNagro left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for taking this on. Left a few comments.

And I appreciate the offer to add a CI. Since they're just a few test dependencies I'm fine doing this manually for now.

build.sbt Outdated Show resolved Hide resolved
"com.dimafeng" %% "testcontainers-scala-munit" % testcontainersVersion % Test,
"com.dimafeng" %% "testcontainers-scala-postgresql" % testcontainersVersion % Test,
"org.postgresql" % "postgresql" % "42.6.0" % Test,
"org.postgresql" % "postgresql" % "42.7.4" % Test,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you also update the docker images to the latest LTS release? For example in PgTests:464

  val pgContainer = ForAllContainerFixture(
    PostgreSQLContainer
      .Def(dockerImageName = DockerImageName.parse("postgres:15.2"))
      .createContainer()
  )

We can use the 17.0 image instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, please also update the docker images for the other Dbs as well:

  • ClickHouseTests:393 Version: 24.3.12.75
  • MySqlTests:395 Version: 8.4.2
  • OracleTests:391 Switch to use https://hub.docker.com/r/gvenzl/oracle-free and version 23.5-faststart (or just 23.5 if that one doesn't work)
  • Also, PgCodecTests:157 in magnum-pg module to 17.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AugustNagro Done :)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, getting

java.lang.IllegalStateException: Failed to verify that image 'gvenzl/oracle-free:23.5-faststart' is a compatible substitute for 'gvenzl/oracle-xe'. This generally means that you are trying to use an image that Testcontainers has not been designed to use. If this is deliberate, and if you are confident that the image is compatible, you should declare compatibility in code using the `asCompatibleSubstituteFor` method. For example:
   DockerImageName myImage = DockerImageName.parse("gvenzl/oracle-free:23.5-faststart").asCompatibleSubstituteFor("gvenzl/oracle-xe");

With the new Oracle image.. would you mind reverting to the earlier gvenzl/oracle-xe? It's not up-to-date with the latest release but there's no rush if test-containers doesn't support it yet.

Copy link
Owner

Choose a reason for hiding this comment

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

MySQL tests are failing for mysql:8.4.3, with

Could not create/start container

Looking at the docker logs, it exits because of a removed option:

unknown option '--skip-host-cache'

This was reported in testcontainers-java and fixed in 1.20.2

However, testcontainers-scala depends on v 1.19.x .

I've filed an issue to upgrade: testcontainers/testcontainers-scala#368

For now, lets just revert the MySQL image upgrade as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AugustNagro Can we merge #53 maybe, please? That will help me detect these stuff

Copy link
Owner

Choose a reason for hiding this comment

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

I reviewed #53 and left a few comments.

Just fyi it's very easy to run the tests locally. You just need to install docker (docker desktop or docker engine), then run sbt test.

build.sbt Show resolved Hide resolved
@guizmaii
Copy link
Contributor Author

And I appreciate the offer to add a CI. Since they're just a few test dependencies I'm fine doing this manually for now.

Hum, I'd prefer if we add a minimal CI running tests.
Without it, it makes it harder for other people to contribute.
Also IMO, it provides more confidence to people interested in using your lib.

@AugustNagro
Copy link
Owner

Hum, I'd prefer if we add a minimal CI running tests.

Yeah CI to run the tests would be very appreciated.

To have something like scala-steward auto-updating the dependencies would be more of a PITA I think. Since I'm not sure scala-steward would be able to update the docker images to the latest LTS releases.

@jivanic-demystdata
Copy link

Let's forget about scala-steward if you don't like it. Let's just add a minimal CI which runs the tests. I'll make another PR for that if that's of for you?

@AugustNagro
Copy link
Owner

Perfect, thanks

@AugustNagro AugustNagro mentioned this pull request Nov 13, 2024
@AugustNagro
Copy link
Owner

Thanks for your work on this @guizmaii

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.

Update test dependency versions
3 participants