Conversation
|
|
|
@jarlah this is the last missing feature for me to complete my library using testcontainers, I'd like to see this in 2.0 (it is a blocker for me). Anything I can help out to get this through? |
|
so sorry.. been awfully busy the last weeks. Actually been a heavy user of testcontainers actually and its not easy to admit that its not as stable as it should be hah |
|
first off the hash test thats because you introduced new struct field in container. So thats easy to fix. Just use the new hash in the test. |
|
ill look into this and the other PR now, plus the AWFUL flakyness of the ryuk connection initialization! Im running two production projects and im not kidding when i say that ryuk fails so many times ai is skipping running tests all together from time to time .. |
|
haha sorry @gossi i destroyed this PR by syncing it. But anway. Master should be extremely more stable now. for ex just see here from a local run, only the periodically 1 in 10 flaky kafka test left its also possible to run tests completely offline. For ex when on a plane and you just want to run a couple of tests when you have all images locally. |
|
im making a new version from this. Actually, i think i will release version 2.0.0-rc2. Then, after using it for a couple of days, checking its more resilient, release 2.0.0 end of week |
|
v2.0.0-rc2 released. Will use it on my two prod apps this coming week |
gossi
left a comment
There was a problem hiding this comment.
The hash fixes were the easy parts ;)
I run into couple others:
- Creating, starting and waiting was (and is) inconsistent (see my comments on ryuk, that reveal that)
- I tried to make the API a bit more explicit (one function each for creating, starting and waiting) - I got quite far I'd say
- I wanted to solve my connreset problem, I fixed the inotify problem
I still have this issue:
1) test copy contents to target (CopyToTest)
Error: test/copy_to_test.exs:5
** (MatchError) no match of right hand side value: {:error, :socket_closed_remotely}
code: {:ok, %{body: body}} = Tesla.get("[http://127.0.0.1:#{mapped_port}/hello.txt](http://127.0.0.1/#{mapped_port}/hello.txt)")
stacktrace:
test/copy_to_test.exs:18: (test)
So, locally when running the tests, it's green and clear. I think this is a race condition when running this on GH actions, although I have no idea which socket is closed (docker, nginx 🤷).
I tried with and without http wait strategy, seems like something else decides to kill some process here.
Anything we can do to make that execution stable?
lib/testcontainers.ex
Outdated
| with {:ok, id} <- create_container(conn, ryuk_config, properties), | ||
| {:ok, container} <- start_container_with_id(conn, id), |
There was a problem hiding this comment.
I'm not sure if I made this better or worse here. In the end ryuk is started as any other container, with the same config, with the same expectation.
Yet it is handled differently.
The weird part, ryuk_config in that situation is used for both config and config_builder - which is a separate thing below o_O. That was (and is) super confusing when I tried to run ryuk and any other container through the same "handlers".
There was a problem hiding this comment.
i would like any changes to this part of the code to be done in a separate PR unless explicitly required by the PRs main changes
lib/testcontainers.ex
Outdated
| defp create_and_start_and_wait_for_container(config, config_builder, state) do | ||
| with {:ok, id} <- create_container(state.conn, config, state.properties), | ||
| {:ok, container} <- start_container_with_id(state.conn, id), | ||
| :ok <- ContainerBuilder.after_start(config_builder, container, state.conn), |
There was a problem hiding this comment.
ie. ryuk doesn't get this after_start check.
There was a problem hiding this comment.
again i think we should add minimal changes. are all these changes required just add copy to container?
|
ha! you made the http strategy stable and my PR is green now - WoW! Please have a look :) |
.github/workflows/elixir.yml
Outdated
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Install inotify-tools |
There was a problem hiding this comment.
is there a specific need for inotify tools ?
There was a problem hiding this comment.
not necessarily. Inotify tools handle file nodes for i/o.
At times I had this as only error message. So, I fixed this at first. (I did the same at my other project using testcontainers). In the end it makes test output cleaner and does not clutter the console output with errors/warnings that detract from actual debugging, that's helpful.
There was a problem hiding this comment.
I gonna extract this.
jarlah
left a comment
There was a problem hiding this comment.
Nice job, :) Sorry about the "thumbs down" but i feel the amount of changes in the core startup logic is too much just for a copy to container logic. Can you scope it down? to just copy to container logic? e.g. as little as changes required to get it working ?
|
there is always a chance i didnt understand the reasoning behind the changes too, so 🐻 that in mind |
Impl for #240
I did some research for the API in rust and node clients:
with_copy_towithCopyContentToContainer()withCopyDirectoriesToContainer()withCopyFilesToContainer()withCopyArchivesToContainer()For elixir, I choose the method from rust
with_copy_toand then use pattern matching against the cases.I personally only need one case here and did the impl for the most basic one 🙈.