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

Adding LogConsumers start as part of the ContainerRequest #2073

Merged
merged 26 commits into from
Jan 25, 2024

Conversation

jespino
Copy link
Contributor

@jespino jespino commented Jan 2, 2024

What does this PR do?

This PR adds the posibility of configuring the LogProducer and the LogConsumers directly in the ContainerRequest config of the container.

Besides, it deprecates the current lifecycle of starting/stopping the log producer, which could lead to errors. Instead, this new approach improves the developer experience providing an internal lifecycle defined by the library: users provide the log consumers and the producer options at the ContainerRequest, and the library will start the consumers when the container starts, stopping them when the container is terminated; so everything is transparent (and simple!).

Finally, we are removing the concept of "log producer", as the container is indeed the "producer" so it seems a duplication that could cause confusion. For that reason, we are updating the docs to talk about "log production", that can be configured, and log consumers. We think it will simplify the mental model for working with log consumers.

Why is it important?

This provides better ergnomoics in the library giving a better understanding of the expected container behavior looking only at the container request, and also provides an easier way to initialize the logging process, without the need of calling the FollowOutput or StartLogProducer methods as separate calls after the creation of the container.

Related issues

How to test this PR

I included a test for this in the code. But if you want to test it manually you need to create a new ContainerRequest that contains the LogConsumers and LogProducer settings, and verify that the LogConsumers are behaving as expected, that means that the log are process since the beginning of the container execution.

Follow-ups

As brilliantly suggested by @ekcasey, we could simply use io.Writer for the consumer, instead of our custom LogConsumer interface, saving one custom type. Another benefit would be leveraging the io.MultiWriter for multiple log consumers.

@jespino I think we could do this in a follow-up PR

@jespino jespino requested a review from a team as a code owner January 2, 2024 08:59
Copy link

netlify bot commented Jan 2, 2024

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit e80650d
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65b12f75b9eb2000088155c0
😎 Deploy Preview https://deploy-preview-2073--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jespino jespino changed the title Adding LogConsumers and LogProducer start as part of the GenericRequest Adding LogConsumers and LogProducer start as part of the ContainerRequest Jan 2, 2024
* main:
  chore: move internal/testcontainersdocker package's files to internal/core (testcontainers#2083)
  GenericContainer: in case of error: return a reference to the failed container (testcontainers#2082)
  [breaking] Add err chan to log producer and don't panic on error (testcontainers#1971)
  chore: enrich HTTP headers to the Docker daemon with the project path (testcontainers#2080)
  fix: align codeql versions in GH workflow (testcontainers#2081)
  chore(deps): bump go.mongodb.org/mongo-driver in /modules/mongodb (testcontainers#2065)
  chore(deps): bump github.com/shirou/gopsutil/v3 from 3.23.11 to 3.23.12 (testcontainers#2068)
  fix(modules.gcloud): pass as ptr to allow request customization (testcontainers#1972)
  chore(deps): bump github.com/twmb/franz-go in /modules/redpanda (testcontainers#2072)
  chore(deps): bump k8s.io/api, k8s.io/apimachinery, k8s.io/client-go from 0.28.4 to 0.29.0 in /modules/k3s (testcontainers#2078)
  chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 (testcontainers#2066)
  chore(deps): bump github.com/google/uuid from 1.4.0 to 1.5.0 (testcontainers#2077)
  bump google.golang.org/api from 0.153.0 to 0.154.0, cloud.google.com/go/spanner from 1.53.1 to 1.54.0, bump google.golang.org/grpc from 1.59.0 to 1.60.1 in /modules/gcloud (testcontainers#2076)
  chore(deps): bump github.com/aws/aws-sdk-go-v2 from 1.23.5 to 1.24.0 (credentials from 1.16.9 to 1.16.13, service/s3 from 1.47.1 to 1.47.7)  in /modules/localstack (testcontainers#2075)
  chore(deps): bump github/codeql-action from 2 to 3 (testcontainers#2056)
  chore(deps): bump test-summary/action from 2.1 to 2.2 (testcontainers#2058)
  chore(deps): bump actions/setup-go from 4 to 5 (testcontainers#2057)
container.go Outdated Show resolved Hide resolved
docker.go Show resolved Hide resolved
docker.go Outdated Show resolved Hide resolved
@mdelapenya mdelapenya self-assigned this Jan 11, 2024
@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Jan 11, 2024
@mdelapenya
Copy link
Member

@Tofel I'd like you to take a look at this one if possible, as you already contributed the log producer options 🙏

@mdelapenya mdelapenya changed the title Adding LogConsumers and LogProducer start as part of the ContainerRequest Adding LogConsumers start as part of the ContainerRequest Jan 12, 2024
* main:
  chore: bring back assertion for network aliases for bridge in rootless mode (testcontainers#2141)
  chore(deps): bump github.com/docker/compose/v2 from 2.23.3 to 2.24.0 in /modules/compose (testcontainers#2096)
docker.go Show resolved Hide resolved
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

As mentioned in the issue, as a follow-up, we can update the log consumer pattern and simply use an io.Writer interface, which will allow to reduce the amount of custom types and also leverage the io.MultiWriter type for having multiple consumers/writers.

@mdelapenya mdelapenya merged commit 16c689a into testcontainers:main Jan 25, 2024
120 checks passed
mdelapenya added a commit to rcrowe/testcontainers-go that referenced this pull request Jan 25, 2024
* main:
  Bump containerd version to v1.7.12 (testcontainers#2137)
  feat: Add Minio module (testcontainers#2132)
  Adding LogConsumers start as part of the ContainerRequest (testcontainers#2073)
  chore: bring back assertion for network aliases for bridge in rootless mode (testcontainers#2141)
  chore(deps): bump github.com/docker/compose/v2 from 2.23.3 to 2.24.0 in /modules/compose (testcontainers#2096)
mdelapenya added a commit to laskoviymishka/testcontainers-go that referenced this pull request Jan 26, 2024
* main:
  Adding inbucket module (testcontainers#2142)
  testifylint: enable compares rule (testcontainers#2143)
  Bump containerd version to v1.7.12 (testcontainers#2137)
  feat: Add Minio module (testcontainers#2132)
  Adding LogConsumers start as part of the ContainerRequest (testcontainers#2073)
  chore: bring back assertion for network aliases for bridge in rootless mode (testcontainers#2141)
  chore(deps): bump github.com/docker/compose/v2 from 2.23.3 to 2.24.0 in /modules/compose (testcontainers#2096)
mdelapenya added a commit to tateexon/testcontainers-go that referenced this pull request Jan 29, 2024
* main: (74 commits)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  chore(deps): bump google.golang.org/api from 0.156.0 to 0.159.0, google.golang.org/grpc from 1.60.1 to 1.61.0, cloud.google.com/go/pubsub from 1.33.0 to 1.35.0 in /modules/gcloud (testcontainers#2168)
  chore(deps): bump github.com/hashicorp/consul/api in /examples/consul (testcontainers#2152)
  chore(deps): bump github.com/couchbase/gocb/v2 in /modules/couchbase (testcontainers#2145)
  chore(deps): bump k8s.io/api, k8s.io/apimachinery and k8s.io/client-go from 0.29.0 to 0.29.1 in /modules/k3s (testcontainers#2167)
  chore: do not compile modules on macos workers on GH (testcontainers#2164)
  Openldap module support (testcontainers#2117)
  Adding inbucket module (testcontainers#2142)
  testifylint: enable compares rule (testcontainers#2143)
  Bump containerd version to v1.7.12 (testcontainers#2137)
  feat: Add Minio module (testcontainers#2132)
  Adding LogConsumers start as part of the ContainerRequest (testcontainers#2073)
  chore: bring back assertion for network aliases for bridge in rootless mode (testcontainers#2141)
  chore(deps): bump github.com/docker/compose/v2 from 2.23.3 to 2.24.0 in /modules/compose (testcontainers#2096)
  chore(deps): bump github.com/dvsekhvalnov/jose2go in /modules/pulsar (testcontainers#2136)
  fix: skip-host-cache option removed in latest MySQL 8.3.0 version (testcontainers#2130)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement]: improve the experience using log producers
2 participants