feat: add oracle image impl #140

DLakomy · 2024-06-01T16:32:45Z

Hopefully everything is ok, but if something could be better, I'll be happy to improve it.

Note that the unit test needs Oracle client to run. I'm not sure how to handle this in the CI pipeline. Unfortunately my experience with GitHub Actions is very limited, so I'll need some tips.

DDtKey

Thank you for the contribution! 🚀

I've left couple of comments, please take a look

Note that the unit test needs Oracle client to run. I'm not sure how to handle this in the CI pipeline. Unfortunately my experience with GitHub Actions is very limited, so I'll need some tips.

I think oracle crate is a good reference for that, we can add installation like this:
https://github.com/kubo/rust-oracle/blob/109acbcc643d0a5bc85a03f5c024e58cebd00ad5/.github/workflows/run-tests.yml#L49-L58 (add this to our test flow)

However, I'm a bit worried about dev experience for contributors 🤔

DDtKey · 2024-06-01T17:22:03Z

src/lib.rs

@@ -63,6 +63,9 @@ pub mod nats;
 #[cfg(feature = "neo4j")]
 #[cfg_attr(docsrs, doc(cfg(feature = "neo4j")))]
 pub mod neo4j;
+#[cfg(feature = "oracle")]
+#[cfg_attr(docsrs, doc(cfg(feature = "oracle")))]
+pub mod oracle;


Probably it makes sense to name the module as oracle_free or something like that. In order to highlight that.
It's pretty common pattern as far as I can see, e.g:

Testcontainers for Java: https://java.testcontainers.org/modules/databases/oraclefree/

Testcontainers for Python: https://github.com/testcontainers/testcontainers-python/tree/main/modules/oracle-free

and couple more not related to testcontainers at all

But the same time we can keep the feature as oracle, because we may expose other images later (e.g XE) 🤔

DDtKey · 2024-06-01T17:26:11Z

src/oracle/mod.rs

+    use super::*;
+    use crate::testcontainers::runners::SyncRunner;
+
+    // remember to provide Oracle client 11.2 or later (see https://crates.io/crates/oracle)


I think it worth mentioning in contributing guide as an optional dependency (in case you need to run oracle tests).

testcontainers-rs-modules-community/CONTRIBUTING.md

Lines 25 to 29 in 071945d

### Setting up local development

- Ensure you have an [up-to-date Rust toolchain](https://rustup.rs/), with `clippy` and `rustfmt` components installed

- Install the [cargo-hack](https://github.com/taiki-e/cargo-hack) subcommand (recommended)

- Fork this repository

Even more, it should mentioned as optional for running oracle test, and if contributor doesn't use ARM

DDtKey · 2024-06-01T17:38:24Z

src/oracle/mod.rs

+/// Module to work with [`Oracle Database`] inside of tests.
+/// The default image is [`gvenzl/oracle-free:23-slim-faststart`] (unofficial).


I see mention about unsupported ARM:

Currently, there is no Oracle Database Free port for ARM chips, hence Oracle Database Free images cannot run on the new Apple M chips via Docker Desktop.

I think this is quite important limitation, so I'd prefer to add cfg with not(any(target_arch="arm", target_arch="aarch64")) for mod oracle_free. Moreover, tests of modules itself won't work at all on ARM-64.

And of course, it's better to mention that as part of documentation.

DLakomy · 2024-06-02T10:59:28Z

Thank you for you comments. To sum up (the most important part is 4.):

oracle_free instead of oracle: theoretically the code would be almost identical for any Oracle database (the default port is always 1521, the ready indicator is always DATABASE IS READY TO USE!). The main difference is the default pdb name (XEPDB1 vs FREEPDB1 in case of free versions), but it's possible to change that to TESTDB to keep the module more universal, via env variable). However, I think it's quite reasonable to just follow the convention from Java/Python, so maybe it's worth to change that to oracle_free, to keep some familiarity with other implementations (it possibly has some other pros as well). If we are both in doubt, maybe just stick to the convention and call it oracle_free.
The target_arch thing: true, no questions here.
I think oracle crate is a good reference for [the CI pipeline] - looks good to me, I can give it a try.
However, I'm a bit worried about dev experience for contributors - actually this remark got me worried too, because it can indeed worsen the experience. It would be the first case where an external binary is needed, right? Here is how I see it: if someone contributes to the Oracle module, they almost for sure already have the Oracle Client libs. So no QOL change in this case. A potential problem is for the contributors who contribute to some other module and run cargo hack test --each-feature --clean-per-run, according to the CONTRIBUTING.md. Maybe suggesting only cargo test -F <the feature you touched> in the guide would suffice 🤔 Anyway, I certainly don't want this contribution to cause problems for the future contributors/maintainers, so please let me know, whether you think the added value is worth it. As far as I know I'm the only person who needed that at the moment (I see no requests in the issue tab 😅) and just wanted to share. If you think it's worth it, I'll start working on the points 1-3. Otherwise I'll be fine with rejection, no hard feelings 😉

BTW I've been looking for alternatives yesterday and today morning, but at the moment I see no mature native driver for Rust, which wouldn't need an external binary. Things are a little easier in Python/Java, there are thin drivers for them. However Python's testcontainers used cx-oracle one day, it involved providing the Oracle Client binary.

DDtKey · 2024-06-02T11:44:49Z

theoretically the code would be almost identical for any Oracle database

Yeah, also thought of it. However, main point here is that we test exactly this image and can't be sure there won't be breaking changes in the future for others

If we are both in doubt, maybe just stick to the convention and call it oracle_free.

I think that's reasonable. At least keep the feature as oracle and rename module to oracle_free. Or even create a submodule: oracle::free.

If we will need to reuse the implementation for another image - that's possible to extend later and reuse the code (some kind of aliases). The only difference is image itself.

But for now, we provide exactly free-db image as a ready and tested module.

A potential problem is for the contributors who contribute to some other module and run cargo hack test --each-feature --clean-per-run, according to the CONTRIBUTING.md. Maybe suggesting only cargo test -F

That sounds about right to me. For contributors it doesn't make a lot of sense to test all modules, but only touched/added.

We always validate all modules in CI anyway. So any side effect should be caught

DDtKey · 2024-06-02T11:48:13Z

As far as I know I'm the only person who needed that at the moment (I see no requests in the issue tab 😅) and just wanted to share

I find this valuable, no doubt.
The absence of existing issues does not mean there is no need. Probably many people just keep their own implementation. Testcontainers have been kinda inactive for a long time.

In such cases I prefer to find a solution instead of avoiding features at all. And I think granularity of features helps here a lot for contributors.

Currently, there is no Oracle Database Free port for ARM chips, hence Oracle Database Free images cannot run on the new Apple M chips via Docker Desktop.

DLakomy · 2024-06-03T15:56:53Z

Hopefully I haven't forgotten about anything (please let me now if I have).

I've added #[cfg(not(any(target_arch = "arm", target_arch = "aarch64")))] to mod.rs, not in lib.rs. The reasoning is that there can be different platform avalability for different images (or maybe arm will use a different image by default, who knows?). I don't insist that it remain this way, though.

Unfortunately CI is still red (a timeout this time). I need to investigate, never had this issue. I've tested the CI before and it worked (https://github.com/DLakomy/testcontainers-rs-modules-community/actions/runs/9351698453). Nonetheless, it wasn't a full CI run, only the Oracle feature was tested. Maybe this is what makes a difference.

EDIT: or maybe Oracle is just too slow and there is nothing we can do about it... It's already slim-faststart image.

DDtKey · 2024-06-03T16:01:38Z

Thank you very much for all efforts!

I see WaitContainer(StartupTimeout) in CI. Probably it takes more than a minute in CI and we can consider increasing the timeout (it's now possible with current version of testcontainers)

DLakomy · 2024-06-03T16:34:15Z

Something like this should do the trick (in oracle_one_plus_one):

let oracle = {
    let image = Oracle::default();
    // TODO find out how long is good enough
    RunnableImage::from(image).with_startup_timeout(Duration::from_secs(90))
};

Tomorrow I'll try to measure the exact time it takes in CI. Maybe something lower than 90 seconds will do.

DDtKey · 2024-06-03T16:40:39Z

We can also consider something like that:

RunnableImage::from(image).pull_image().unwrap().start().unwrap()

Just in case if it requires a lot of time to pull the image

DLakomy · 2024-06-04T16:38:36Z

Hopefully it will be green this time. At least stable. I've noticed that Postgres is failing nightly during my tests.

I've measured the time here: https://github.com/DLakomy/testcontainers-rs-modules-community/actions/runs/9369470735.

The results are (seconds; three attempts; nightly vs stable ofc makes no difference, just another sample):

Pull	Start	Type
39	52	stable
40	56	nightly
39	53	stable
40	56	nightly
41	54	stable
41	54	nightly

DDtKey · 2024-06-04T17:16:08Z

I've noticed that Postgres is failing nightly during my tests.

I guess some images are kinda flaky in CI right now because of timeout. Sometimes 1 minute is just not enough for pulling + startup. But usually it works perfectly fine

I'll observe the behavior over time and likely adjust the timeouts for necessary images for CI.

DDtKey

Thank you!

DLakomy · 2024-06-04T17:55:47Z

Thank you. Note that it wasn't a timeout this time, in case of Postgres.

From the last run:

  ---- src/mysql/mod.rs - mysql::Mysql (line 15) stdout ----
  thread 'rustc' panicked at compiler/rustc_codegen_ssa/src/back/link.rs:2700:27:
  index out of bounds: the len is 133 but the index is 133

(mysql this time; something really strange is happening with this nightly, maybe it needs another night...)

DDtKey · 2024-06-04T18:25:46Z

Yes, I see
That's definitely a bug in nightly version of rustc

## 🤖 New release * `testcontainers-modules`: 0.5.0 -> 0.5.1 <details><summary>Changelog</summary> <blockquote> ## [0.5.1] - 2024-06-14 ### Documentation - Fix typo ([#141](#141)) ### Features - Add oracle image impl ([#140](#140)) - Add `clickhouse` image ([#142](#142))  </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

feat: add oracle image impl

0b47af5

DDtKey reviewed Jun 1, 2024

View reviewed changes

DLakomy added 4 commits June 2, 2024 17:56

refactor(oracle): Create dedicated Oracle Free submodule

025e106

chore(oracle): Disable Oracle feature for ARM

c5290a5

Currently, there is no Oracle Database Free port for ARM chips, hence Oracle Database Free images cannot run on the new Apple M chips via Docker Desktop.

docs: Update CONTRIBUTING.md (Oracle setup)

7b4ee30

ci(deps): add oracle client library

2e68048

ci(oracle): increase oracle startup timeout

727dce4

DDtKey approved these changes Jun 4, 2024

View reviewed changes

DDtKey merged commit 27fcd4e into testcontainers:main Jun 4, 2024
5 of 6 checks passed

github-actions bot mentioned this pull request Jun 14, 2024

chore: release v0.5.1 #145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add oracle image impl #140

feat: add oracle image impl #140

DLakomy commented Jun 1, 2024

DDtKey left a comment •

edited

Loading

DDtKey Jun 1, 2024

DDtKey Jun 1, 2024

DDtKey Jun 1, 2024

DDtKey Jun 1, 2024

DLakomy commented Jun 2, 2024

DDtKey commented Jun 2, 2024 •

edited

Loading

DDtKey commented Jun 2, 2024 •

edited

Loading

DLakomy commented Jun 3, 2024 •

edited

Loading

DDtKey commented Jun 3, 2024 •

edited

Loading

DLakomy commented Jun 3, 2024

DDtKey commented Jun 3, 2024

DLakomy commented Jun 4, 2024

DDtKey commented Jun 4, 2024 •

edited

Loading

DDtKey left a comment

DLakomy commented Jun 4, 2024

DDtKey commented Jun 4, 2024 •

edited

Loading

	### Setting up local development

	- Ensure you have an [up-to-date Rust toolchain](https://rustup.rs/), with `clippy` and `rustfmt` components installed
	- Install the [cargo-hack](https://github.com/taiki-e/cargo-hack) subcommand (recommended)
	- Fork this repository

		/// Module to work with [`Oracle Database`] inside of tests.
		/// The default image is [`gvenzl/oracle-free:23-slim-faststart`] (unofficial).

feat: add oracle image impl #140

feat: add oracle image impl #140

Conversation

DLakomy commented Jun 1, 2024

DDtKey left a comment • edited Loading

Choose a reason for hiding this comment

DDtKey Jun 1, 2024

Choose a reason for hiding this comment

DDtKey Jun 1, 2024

Choose a reason for hiding this comment

DDtKey Jun 1, 2024

Choose a reason for hiding this comment

DDtKey Jun 1, 2024

Choose a reason for hiding this comment

DLakomy commented Jun 2, 2024

DDtKey commented Jun 2, 2024 • edited Loading

DDtKey commented Jun 2, 2024 • edited Loading

DLakomy commented Jun 3, 2024 • edited Loading

DDtKey commented Jun 3, 2024 • edited Loading

DLakomy commented Jun 3, 2024

DDtKey commented Jun 3, 2024

DLakomy commented Jun 4, 2024

DDtKey commented Jun 4, 2024 • edited Loading

DDtKey left a comment

Choose a reason for hiding this comment

DLakomy commented Jun 4, 2024

DDtKey commented Jun 4, 2024 • edited Loading

DDtKey left a comment •

edited

Loading

DDtKey commented Jun 2, 2024 •

edited

Loading

DDtKey commented Jun 2, 2024 •

edited

Loading

DLakomy commented Jun 3, 2024 •

edited

Loading

DDtKey commented Jun 3, 2024 •

edited

Loading

DDtKey commented Jun 4, 2024 •

edited

Loading

DDtKey commented Jun 4, 2024 •

edited

Loading