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: Replace output consumer with IContainer.GetLogsAsync(DateTime, DateTime, bool, CancellationToken) #793

Merged
merged 6 commits into from
Feb 19, 2023

Conversation

HofmeisterAn
Copy link
Collaborator

What does this PR do?

This pull request sets the WithOutputConsumer(IOutputConsumer) member obsolete, as it is no longer necessary to attach a stream to consume the output of a container. Instead, getting stdout or stderr is much easier now, as developers can simply call IContainer.GetLogsAsync(DateTime, DateTime, bool, CancellationToken).

Why is it important?

This PR is an important step in continuing the clean up of the API, as it further reduces the amount of configuration necessary to get log messages.

Related issues

-

@HofmeisterAn HofmeisterAn added the enhancement New feature or request label Feb 19, 2023
@netlify
Copy link

netlify bot commented Feb 19, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 0c72125
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/63f1db8cc4b7c30008d08ef6
😎 Deploy Preview https://deploy-preview-793--testcontainers-dotnet.netlify.app/api/create_docker_container
📱 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 settings.

@HofmeisterAn HofmeisterAn merged commit d3ae00f into develop Feb 19, 2023
@HofmeisterAn HofmeisterAn deleted the feature/remove-wait-strategy-log-message-stream branch February 19, 2023 18:19
HofmeisterAn added a commit that referenced this pull request Feb 27, 2023
@@ -385,13 +397,10 @@ protected virtual async Task UnsafeStartAsync(CancellationToken ct = default)
await this.UnsafeCreateAsync(ct)
.ConfigureAwait(false);

await this.client.AttachAsync(this.container.ID, this.configuration.OutputConsumer, ct)

Choose a reason for hiding this comment

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

This was such a useful feature. Don't understand why it got removed. In my case I use it to write to xunit's ITestOutputHelper and can see logs in IDE at runtime. Now the only option is to run a background task that will periodically call GetLogsAsync. For now, as a workaround to preserve the previous behavior, I override UnsafeStartAsync and call AttachAsync via reflection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intention is explained in the pull request. We have noticed that developers frequently use the consumer wrong. Using the GetLogsAsync(DateTime, DateTime, bool, CancellationToken) method is much easier. However, please create an issue and include your use case in it, and we will consider adding the attach call again. Having both is probably not a big deal. Thanks for bringing this up.

@jacobjmarks
Copy link
Contributor

I second that this deprecation seems strange. I am looking to redirect output from the container on an "as-written" basis; I do not simply want to grab logs on an ad-hoc basis via a point-in-time method call. As far as I can tell, this log streaming use case is not supported without the Output Consumer API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants