Skip to content

Conversation

@the-glu
Copy link
Contributor

@the-glu the-glu commented Aug 13, 2025

Add datastore unit tests to ensure various DatastoreDBNode works against real database.

Database are spawn using testcontainer, I added DiD support to start thoses containers.

I fixed is_reachable for yugabyte that require hashed password in some cases.

I used a custom image that I build for cockroachdb with tls 1.1 support, I didn't find old images (2017..). I suppose we want to upload that on the official interuss account?

Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

That's really great, thanks!

I used a custom image that I build for cockroachdb with tls 1.1 support, I didn't find old images (2017..). I suppose we want to upload that on the official interuss account?

Indeed. And actually I'd go as far as uploading all images used in this test in the interuss Docker Hub repo. WDYT @BenjaminPelletier ? We would need you to do so :)

But that should not prevent this PR from going in IMO.

{ url = "https://files.pythonhosted.org/packages/4e/8c/f3147f5c4b73e7550fe5f9352eaa956ae838d5c51eb58e7a25b9f3e2643b/decorator-5.2.1-py3-none-any.whl", hash = "sha256:d316bb415a2d9e2d2b3abcc4084c6502fc09240e292cd76a76afc106a1c8e04a", size = 9190, upload-time = "2025-02-24T04:41:32.565Z" },
]

[[package]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this prevent the project from being run on some OSs? Or is it transparent whether we are on Linux, MacOS or Windows?

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, shouldn't this be added as a dev dependency to avoid users having to depend on this?
(or maybe it is already the case and I did not notice it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that a dev dependency, I added in the wrong group ^^'

For OS that should be transparent*, and in all cases unit tests run in a controller environment in a docker container by default, so that shouldn't interfere :)

  • As long as we can start docker containers ^^'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we don't have a dev group yet, but there are other things that should go into that 'group', I suggest we postpone the fix in a future PR, with two separate groups and two image, one for 'tooling' and a light one with only running dependencies installed :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@the-glu SGTM, if you don't do it in the short-term, could you open an issue about it?

@BenjaminPelletier
Copy link
Member

I used a custom image that I build for cockroachdb with tls 1.1 support, I didn't find old images (2017..). I suppose we want to upload that on the official interuss account?

Indeed. And actually I'd go as far as uploading all images used in this test in the interuss Docker Hub repo. WDYT @BenjaminPelletier ? We would need you to do so :)

interuss/insecurecockroach should now be available (mirrored from mcuoorb). If we use this image, then I think :latest is fine since we control updates. But for any external image, I would expect us to pin to a specific version to avoid unexpected breakages when the remote changes.

I would expect us to follow whatever we document for users for the other images. We could consider mirroring images of the specifically-supported underlying database technologies via the interuss Docker org, but that would be a separate discussion. Until we decide otherwise, I think our standard procedure has our users use the original images so we should use those also.

@mickmis
Copy link
Contributor

mickmis commented Aug 18, 2025

FTR: #1094 has been merged while it contained the commits present in this PR. This PR now contains only the fixes that happened in between: change in uv.lock and use of interuss docker hub repo.

@mickmis mickmis merged commit 8a35b31 into interuss:main Aug 18, 2025
21 checks passed
@the-glu the-glu deleted the test_psycopg branch August 18, 2025 08:27
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