Merged
Conversation
Reviewer's GuideAdds standardized Testcontainers marker labels (including a shared session ID) to containers and networks, centralizes label definitions in DockerClient, and extends tests to verify label application and consistency across resources. Sequence diagram for Docker network creation with default labelssequenceDiagram
participant Network
participant DockerClient
participant DocrAPI
participant DockerEngine
Network->>DockerClient: default_labels()
DockerClient-->>Network: Hash(String, String)
Network->>DockerClient: api()
DockerClient-->>Network: DocrAPI
Network->>DocrAPI: networks.create(name, driver, check_duplicate, labels)
DocrAPI->>DockerEngine: HTTP POST /networks/create with labels
DockerEngine-->>DocrAPI: network_id
DocrAPI-->>Network: network_id
Network->>Network: store network_id
Class diagram for Testcontainers Docker label handlingclassDiagram
class Testcontainers {
}
class DockerClient {
<<module>>
+DocrAPI api
+String TESTCONTAINERS_LABEL
+String TESTCONTAINERS_SESSION_ID_LABEL
+String TESTCONTAINERS_LANG_LABEL
+String TESTCONTAINERS_VERSION_LABEL
+String SESSION_ID
+get : DocrAPI
+reset : Nil
+default_labels() Hash~String, String~
+host() String
}
class DockerContainer {
+start() Nil
+stop() Nil
-default_labels() Hash~String, String~
}
class Network {
+String name
+String driver
+String network_id
+Hash~String, String~ labels
+create() Nil
}
Testcontainers <|-- DockerClient
Testcontainers <|-- DockerContainer
Testcontainers <|-- Network
DockerContainer ..> DockerClient : uses_default_labels
Network ..> DockerClient : uses_default_labels
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds centralized default labels (including a session ID, language, version, and marker) in DockerClient and applies them to containers and networks; tests added to verify labels on DockerContainer and Network, and an integration test checks labels are present on both container and network. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
integration_spec.cr,network_labels = network.info.labelsis used without a nil check (unlikecontainer_labels), so consider asserting non-nil or usingnot_nil!consistently before indexing intonetwork_labelsto avoid potential crashes if labels are ever missing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `integration_spec.cr`, `network_labels = network.info.labels` is used without a nil check (unlike `container_labels`), so consider asserting non-nil or using `not_nil!` consistently before indexing into `network_labels` to avoid potential crashes if labels are ever missing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/testcontainers/docker_client.cr (1)
1-41:⚠️ Potential issue | 🟡 MinorFix Crystal formatting to resolve CI failure.
The pipeline indicates that
crystal tool formatproduces changes for this file. The hash literal alignment in thedefault_labelsmethod (lines 36-39) likely uses inconsistent whitespace padding that the formatter will normalize.Run
crystal tool format ./src/testcontainers/docker_client.crto fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/docker_client.cr` around lines 1 - 41, The hash literal in DockerClient.default_labels has inconsistent whitespace/padding that fails crystal tool format; run `crystal tool format` on the file (or reformat the hash so keys and values are aligned per Crystal style) to normalize spacing around the hash braces and between keys/values for TESTCONTAINERS_LABEL, TESTCONTAINERS_SESSION_ID_LABEL, TESTCONTAINERS_LANG_LABEL and TESTCONTAINERS_VERSION_LABEL (or simply run `crystal tool format ./src/testcontainers/docker_client.cr`) and commit the formatted change.
🧹 Nitpick comments (1)
src/testcontainers/network.cr (1)
22-22: Consider adding a doc comment for thelabelsgetter.As per coding guidelines, all public APIs in
src/**/*.crshould have#doc comments. A brief description would help users understand this property.📝 Suggested doc comment
getter network_id : String? + # Returns the labels applied to this network. getter labels : Hash(String, String)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/network.cr` at line 22, Add a Crystal doc comment for the public getter `labels` in src/testcontainers/network.cr: describe what the `labels : Hash(String, String)` property represents (e.g., network metadata/labels used for identification or Docker labels), any important behaviour or constraints, and an example or note if relevant; place the comment immediately above the `getter labels` declaration so it appears in generated docs for the `labels` getter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/testcontainers/docker_client.cr`:
- Around line 1-41: The hash literal in DockerClient.default_labels has
inconsistent whitespace/padding that fails crystal tool format; run `crystal
tool format` on the file (or reformat the hash so keys and values are aligned
per Crystal style) to normalize spacing around the hash braces and between
keys/values for TESTCONTAINERS_LABEL, TESTCONTAINERS_SESSION_ID_LABEL,
TESTCONTAINERS_LANG_LABEL and TESTCONTAINERS_VERSION_LABEL (or simply run
`crystal tool format ./src/testcontainers/docker_client.cr`) and commit the
formatted change.
---
Nitpick comments:
In `@src/testcontainers/network.cr`:
- Line 22: Add a Crystal doc comment for the public getter `labels` in
src/testcontainers/network.cr: describe what the `labels : Hash(String, String)`
property represents (e.g., network metadata/labels used for identification or
Docker labels), any important behaviour or constraints, and an example or note
if relevant; place the comment immediately above the `getter labels` declaration
so it appears in generated docs for the `labels` getter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21f65570-7b97-4cb0-9a8f-32472d38c6f1
📒 Files selected for processing (6)
spec/docker_container_spec.crspec/integration_spec.crspec/network_spec.crsrc/testcontainers/docker_client.crsrc/testcontainers/docker_container.crsrc/testcontainers/network.cr
dragosv
force-pushed
the
testcontainers-labels
branch
from
eb8e0d2 to
1e35dd1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
==========================================
+ Coverage 58.50% 59.35% +0.84%
==========================================
Files 14 14
Lines 335 342 +7
==========================================
+ Hits 196 203 +7
Misses 139 139
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/testcontainers/network.cr (1)
22-22: Consider adding a doc comment for thelabelsgetter.Per coding guidelines, public APIs should have
#doc comments. A brief description of the labels' purpose and structure would help users understand this property.+ # Returns the metadata labels applied to this network. getter labels : Hash(String, String)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/network.cr` at line 22, Add a RubyCrystal doc comment above the getter `labels : Hash(String, String)` (use a `#` comment immediately above the declaration) that briefly explains this property holds the network's label metadata as string key/value pairs, describes the intended use (e.g., container/network metadata or Docker labels), and notes the expected format (Hash(String, String)). Reference the `labels` getter when adding the comment so users can see purpose and structure in the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/testcontainers/network.cr`:
- Line 22: Add a RubyCrystal doc comment above the getter `labels : Hash(String,
String)` (use a `#` comment immediately above the declaration) that briefly
explains this property holds the network's label metadata as string key/value
pairs, describes the intended use (e.g., container/network metadata or Docker
labels), and notes the expected format (Hash(String, String)). Reference the
`labels` getter when adding the comment so users can see purpose and structure
in the public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 247c84a4-2c32-4b1f-aca7-e6a0d3043fbd
📒 Files selected for processing (6)
spec/docker_container_spec.crspec/integration_spec.crspec/network_spec.crsrc/testcontainers/docker_client.crsrc/testcontainers/docker_container.crsrc/testcontainers/network.cr
🚧 Files skipped from review as they are similar to previous changes (4)
- src/testcontainers/docker_client.cr
- src/testcontainers/docker_container.cr
- spec/integration_spec.cr
- spec/network_spec.cr
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement full Testcontainers label specification
Summary
Implements the standard Testcontainers label specification across all Docker resources (containers and networks), aligning with the conventions established by the Java and Ruby reference implementations.
Problem
Only two labels were applied to containers (
org.testcontainers.langandorg.testcontainers.version), and none were applied to networks. The official Testcontainers specification requires four labels on every resource, including theorg.testcontainersmarker and a per-processsessionIdused by the Ryuk resource reaper for cleanup.Changes
src/testcontainers/docker_client.crTESTCONTAINERS_LABEL,TESTCONTAINERS_SESSION_ID_LABEL,TESTCONTAINERS_LANG_LABEL,TESTCONTAINERS_VERSION_LABELSESSION_ID— a per-process UUID shared across all resourcesDockerClient.default_labelsreturning the full canonical label setsrc/testcontainers/docker_container.crdefault_labelsnow delegates toDockerClient.default_labelssrc/testcontainers/network.crlabelsgetter initialized fromDockerClient.default_labelsDocr::Types::NetworkConfigon network creationspec/docker_container_spec.crorg.testcontainers,sessionId,lang,version)spec/network_spec.crspec/integration_spec.crLabels applied to resources
org.testcontainers"true"org.testcontainers.sessionIdorg.testcontainers.lang"crystal"org.testcontainers.versionTestcontainers::VERSIONTesting
Summary by Sourcery
Add standardized Testcontainers metadata labels to Docker containers and networks and ensure they are applied consistently across resources within a session.
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests