Skip to content

feat: make port mapping checks timeout configurable #3176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mdelapenya wants to merge 5 commits into from
Closed

feat: make port mapping checks timeout configurable #3176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f1415e6
feat: make exposing ports configurable by config
mdelapenya May 30, 2025
e6ea364
chore: apply configuration to the port mapping check
mdelapenya May 30, 2025
c603baf
chore: refactor checkPortsMapped to make it testable
mdelapenya May 30, 2025
240b622
chore: refactor config tests for maintainability
mdelapenya May 30, 2025
613b0ab
chore: enforce value at CI
mdelapenya May 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/ci-test-go.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ jobs:
env:
TESTCONTAINERS_RYUK_DISABLED: "${{ inputs.ryuk-disabled }}"
RYUK_CONNECTION_TIMEOUT: "${{ inputs.project-directory == 'modules/compose' && '5m' || '60s' }}"
TESTCONTAINERS_PORT_MAPPING_TIMEOUT: "5s"
Copy link
Contributor

@stevenh stevenh May 30, 2025

Choose a reason for hiding this comment

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

question: In the standard case 5s might sound generous but if a system is underload this could cause issues, what lead to choice of 5s as the default?

RYUK_RECONNECTION_TIMEOUT: "${{ inputs.project-directory == 'modules/compose' && '30s' || '10s' }}"
SHOULD_RUN_SONAR: "false"
strategy:
Expand Down
10 changes: 10 additions & 0 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@ type Config struct {
//
// Environment variable: TESTCONTAINERS_DOCKER_SOCKET_OVERRIDE
TestcontainersHost string `properties:"tc.host,default="`

// TestcontainersPortMappingTimeout is the time to wait before all the exposed ports are mapped.
//
// Environment variable: TESTCONTAINERS_PORT_MAPPING_TIMEOUT
TestcontainersPortMappingTimeout time.Duration `properties:"tc.port.mapping.timeout,default=5s"`
}

// }
Expand Down Expand Up @@ -141,6 +146,11 @@ func read() Config {
config.RyukConnectionTimeout = timeout
}

testcontainersPortMappingTimeoutEnv := readTestcontainersEnv("TESTCONTAINERS_PORT_MAPPING_TIMEOUT")
Copy link
Contributor

@mabrarov mabrarov May 30, 2025

Choose a reason for hiding this comment

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

This value is used in defaultReadinessHook. What if this hook will be extended in future and will include more checks than just check of the port mapping? "Port mapping" (in the name of environment variable, in the name of local variable and in the name of field of Config struct) sounds like exposure of implementation details of "default readiness hook".

What if we use different name (I'm not good in naming), like:

Suggested change
testcontainersPortMappingTimeoutEnv := readTestcontainersEnv("TESTCONTAINERS_PORT_MAPPING_TIMEOUT")
testcontainersDefaultReadinessHookTimeoutEnv := readTestcontainersEnv("TESTCONTAINERS_DEFAULT_READINESS_HOOK_TIMEOUT")

?

if timeout, err := time.ParseDuration(testcontainersPortMappingTimeoutEnv); err == nil {
config.TestcontainersPortMappingTimeout = timeout
}

return config
}

Expand Down
Loading
Loading