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.

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: log docker info from compose #591

Merged
merged 3 commits into from Oct 28, 2022

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Oct 27, 2022

What is this PR doing?

This PR extracts the function to log the docker info command from the provider to a helper method, so that it can be reused when creating a compose instance.

This new func recieves a Go context, a Docker client and the logger to be used

Finally, I noticed the existing logServerInfo function was incorrect in the flow: if an error occurred while getting the Info, the code did not exit the function, continuing with a wrong and empty message.

Why is it important?

We added the log info for the generic containers, but did not add it to the compose ones. After #476, now it's possible to spin up native compose containers from Go, without shelling out to the local binary, and this log was missing (the log was added way later than this PR was born)

Related issues

Follow ups

As part of #549, where we want to initialise resources once, we could improve the architectural design to have a single docker client for both container types (compose and generic), therefore we could simply log the server info in one single place, not in two as we have now.

@mdelapenya mdelapenya requested a review from a team as a code owner October 27, 2022 15:23
@mdelapenya mdelapenya added compose Docker Compose. enhancement New feature or request labels Oct 27, 2022
@mdelapenya mdelapenya self-assigned this Oct 27, 2022
if err != nil {
p.Logger.Printf("failed getting information about docker server: %s", err)
logger.Printf("failed getting information about docker server: %s", err)
return
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed this if was incomplete, as it should exit the function.

I wonder if we should raise an error here, as if the docker info failed, then we cannot continue. In the meantime, we can live with the return, as this function is called after the docker client creation, so it's expected using the client is healthy

@mdelapenya mdelapenya changed the title chore: log docker info from compose feat: log docker info from compose Oct 28, 2022
kiview
kiview approved these changes Oct 28, 2022
Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

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

LGTM, the follow up idea sounds like a good plan 👍

@@ -22,6 +23,7 @@ const (
envComposeFile = "COMPOSE_FILE"
)

var composeLogOnce sync.Once
Copy link
Member

Choose a reason for hiding this comment

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

oh, such a cool feature in Go 😮

@mdelapenya mdelapenya merged commit ab2f0be into testcontainers:main Oct 28, 2022
14 checks passed
@mdelapenya mdelapenya added feature New functionality or new behaviors on the existing one and removed enhancement New feature or request labels Oct 28, 2022
@mdelapenya mdelapenya deleted the compose-log-info branch October 31, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compose Docker Compose. feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants