Skip to content
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

feat(ryuk): make listen address of exposed port configurable #2809

Closed
wants to merge 3 commits into from
Closed

feat(ryuk): make listen address of exposed port configurable #2809

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
937f3b5
feat(ryuk): wait on Started log
romainlaurent Oct 3, 2024
755f116
fix(ryul): test by overriding reaper endpoint
romainlaurent Oct 3, 2024
5b08390
feat(ryuk): use an external ryuk
romainlaurent Oct 3, 2024
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
22 changes: 5 additions & 17 deletions reaper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,22 +204,10 @@ func reuseReaperContainer(ctx context.Context, sessionID string, provider Reaper

Logger.Printf("⏳ Waiting for Reaper port to be ready")

var containerJson *types.ContainerJSON

if containerJson, err = reaperContainer.Inspect(ctx); err != nil {
return nil, fmt.Errorf("failed to inspect reaper container %s: %w", reaperContainer.ID[:8], err)
}

if containerJson != nil && containerJson.NetworkSettings != nil {
for port := range containerJson.NetworkSettings.Ports {
err := wait.ForListeningPort(port).
WithPollInterval(100*time.Millisecond).
WaitUntilReady(ctx, reaperContainer)
if err != nil {
return nil, fmt.Errorf("failed waiting for reaper container %s port %s/%s to be ready: %w",
reaperContainer.ID[:8], port.Proto(), port.Port(), err)
}
}
err = wait.ForLog("Started").WaitUntilReady(ctx, reaperContainer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

bug: If the listening for doesn't work the container isn't accessible so wont work.

if err != nil {
return nil, fmt.Errorf("failed waiting for reaper container %s to be ready: %w",
reaperContainer.ID[:8], err)
}

return &Reaper{
Expand Down Expand Up @@ -249,7 +237,7 @@ func newReaper(ctx context.Context, sessionID string, provider ReaperProvider) (
ExposedPorts: []string{string(listeningPort)},
Labels: core.DefaultLabels(sessionID),
Privileged: tcConfig.RyukPrivileged,
WaitingFor: wait.ForListeningPort(listeningPort),
WaitingFor: wait.ForLog("Started"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

bug: If the listening for check doesn't work then the reaper wont be accessible so this is not a good fix.

Copy link
Author

Choose a reason for hiding this comment

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

It could work by adding TESTCONTAINERS_RYUK_ENDPOINT_OVERRIDE_BY_NAME=true, allowing us to contact the container directly through its name.
I'm not sure it's the most elegant solution, so I'm open to any alternatives you think would be better.

Name: reaperContainerNameFromSessionID(sessionID),
HostConfigModifier: func(hc *container.HostConfig) {
hc.AutoRemove = true
Expand Down