Support Container.with_privileged for Ryuk Privileged Mode
#221
Conversation
1. Environment variables (TESTCONTAINERS_* prefix) 2. User file (~/.testcontainers.properties) 3. Project file (.testcontainers.properties)
Parse properties earlier to allow controlling Ryuk container config, specifically supporting the `with_privileged` flag.
There was a problem hiding this comment.
Pull request overview
This PR adds support for running Ryuk (the Testcontainers cleanup utility) in privileged mode, which is needed for Podman on macOS where SELinux blocks socket access.
Key changes:
- Adds
read_property_sources/1to PropertiesParser to read configuration from multiple sources (environment variables, user file, and project file) with proper precedence - Implements
Container.with_privileged/2to set privileged mode on containers - Updates Testcontainers setup to read properties before configuring Ryuk and applies the privileged setting from
ryuk.container.privilegedproperty
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/container.ex | Adds with_privileged/2 function to enable setting privileged mode on containers |
| lib/util/properties.ex | Implements read_property_sources/1 to read properties from environment variables, user file, and project file with proper precedence; adds helper functions for file reading and environment variable conversion |
| lib/testcontainers.ex | Modifies setup to read properties before Ryuk configuration and applies the privileged setting from properties |
| test/container_test.exs | Adds tests for with_privileged/2 functionality |
| test/util/properties_test.exs | Adds comprehensive tests for the new PropertiesParser functionality including environment variable reading, file reading, and precedence handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test "reads environment variables with TESTCONTAINERS_ prefix" do | ||
| System.put_env("TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED", "true") | ||
| System.put_env("TESTCONTAINERS_SOME_OTHER_PROPERTY", "value") | ||
|
|
||
| {:ok, props} = PropertiesParser.read_property_sources() | ||
|
|
||
| assert props["ryuk.container.privileged"] == "true" | ||
| assert props["some.other.property"] == "value" | ||
|
|
||
| # Cleanup | ||
| System.delete_env("TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED") | ||
| System.delete_env("TESTCONTAINERS_SOME_OTHER_PROPERTY") | ||
| end |
There was a problem hiding this comment.
The test lacks proper cleanup if an assertion fails. Use ExUnit's on_exit/1 callback or setup/teardown to ensure environment variables are always cleaned up, even when tests fail. This prevents test isolation issues where a failing test could pollute the environment for subsequent tests.
| test "environment variables take precedence over file properties" do | ||
| # Set env var | ||
| System.put_env("TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED", "from_env") | ||
|
|
||
| {:ok, props} = PropertiesParser.read_property_sources() | ||
|
|
||
| # Env should win over any file-based setting | ||
| assert props["ryuk.container.privileged"] == "from_env" | ||
|
|
||
| # Cleanup | ||
| System.delete_env("TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED") | ||
| end |
There was a problem hiding this comment.
The test lacks proper cleanup if an assertion fails. Use ExUnit's on_exit/1 callback or setup/teardown to ensure environment variables are always cleaned up, even when tests fail.
| - `:project_file` - path to project properties file (default: .testcontainers.properties) | ||
| - `:env_prefix` - environment variable prefix (default: TESTCONTAINERS_) | ||
| Returns `{:ok, map}` with merged properties. |
There was a problem hiding this comment.
The documentation should follow the project's standard format by using a proper "## Returns" section header instead of inline text. This maintains consistency with other documented functions in the codebase (see lib/testcontainers.ex and lib/ecto.ex for examples).
| Returns `{:ok, map}` with merged properties. | |
| ## Returns | |
| - `{:ok, map}` with merged properties. |
| test "returns empty map when no files or env vars exist" do | ||
| # Clean env vars that might interfere | ||
| System.get_env() | ||
| |> Enum.filter(fn {k, _} -> String.starts_with?(k, "TESTCONTAINERS_") end) | ||
| |> Enum.each(fn {k, _} -> System.delete_env(k) end) | ||
|
|
||
| {:ok, props} = PropertiesParser.read_property_sources() | ||
|
|
||
| # Should at least return a map (may have project file props) | ||
| assert is_map(props) | ||
| end |
There was a problem hiding this comment.
The test name claims "returns empty map when no files or env vars exist" but the test doesn't ensure no files exist and doesn't verify the map is empty. The comment on line 15 acknowledges this by noting "may have project file props". Consider either renaming the test to accurately describe what it tests (e.g., "returns a map with merged properties from all sources") or modify the test to actually verify an empty map when no sources exist by mocking the file system and environment.
|
Is this containing the same properties parser / code as the other pr ? |
It does, but only because it's forked from the other branch. Sorry for that confusion. Original intention was to split them as they're technically two separate changes. But of course, my late evening shuffle botched that. |
jarlah
left a comment
jarlah
left a comment
There was a problem hiding this comment.
I cant see any obvious wrongdoings other than what copilot has mentioned. I suggest that you look into copilots comments, especially about doc strings, test name and test cleanup. Other than that you could maybe also look into removing the read_property_file function completely if not used anymore.
|
Nice work btw. Appreciate your contribution. 🤌 |
You're welcome, thanks for your efforts here as well! Getting caught up from the holidays here, but I will wrap up the feedback and get an update pushed here as soon as I can. |
2 participants
Enables running Ryuk in privileged mode via
ryuk.container.privileged=true. Needed for Podman on macOS where SELinux blocks socket access.with_privileged/2to Container