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

Logrus buffer issue #1511

Merged
merged 11 commits into from
May 6, 2020
Merged

Logrus buffer issue #1511

merged 11 commits into from
May 6, 2020

Conversation

stippi2
Copy link
Member

@stippi2 stippi2 commented May 5, 2020

Changes

Provides work-around for #1474

  • Implement an io.Writer to be used instead of Entry().Writer() for re-routing a tool's output to the logging framework.
  • Replace any occurrence of log.Entry().Writer() with log.Writer().

This avoids the problem of writing chunks larger than 64K that contain no linebreaks. Note that this is not a general solution, as it only solves re-directing tool output to the logging framework. Logrus doesn't provide any facility to override the scanning function. Also it doesn't prevent someone from using log.Entry().Writer(). I am open to other ideas.

  • Tests
  • Documentation - does not apply

@@ -178,7 +178,7 @@ func readCfServiceKey(config abapEnvironmentPullGitRepoOptions, c execRunner) (s

var abapServiceKey serviceKey

c.Stderr(log.Entry().Writer())
c.Stderr(log.Writer())
Copy link
Contributor

Choose a reason for hiding this comment

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

@stippi2 would it make sense to provide a convenience method as part of command.go like c.StderrToLog() and similarly c.StdoutToLog(). I know it potentially spoils the interface but could reduce the risk of someone falling into using log.Entry().Writer(). What do you think?

Copy link
Member Author

@stippi2 stippi2 May 6, 2020

Choose a reason for hiding this comment

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

This is what auto-completion currently gives:
image
... but of course you would have to know to look in log in the first place. I think your proposal makes sense, but what do you think about providing a function returning a command instance, which is already fully configured instead (or in addition)?

Copy link
Contributor

@nevskrem nevskrem May 6, 2020

Choose a reason for hiding this comment

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

@stippi2 good to see that auto-completion proposes the right things;-). Up to now we do already pre-configure the redirect as part of the construction of command.

The methods would more be for scenarios were i.e. Stdout needs temporary redirection perhaps to capture command output and then afterwards reset it and send it back to the logger.

I'd also be fine to do it in an agile manner, continue as is and optimize based on future demand. Still fairly easy to extend and change that later on.

Copy link
Member

Choose a reason for hiding this comment

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

command.go like c.StderrToLog() and similarly c.StdoutToLog(). I know it potentially spoils the interface

I personally would prefer not keep the interfaces small. In case somebody would like to use another writer etc ... it can simply be provide at c.stderr(MY_WRITER). Less benefit added IMO with additional methods ...

@stippi2
Copy link
Member Author

stippi2 commented May 6, 2020

/it

Copy link
Member

@OliverNocon OliverNocon left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@stippi2
Copy link
Member Author

stippi2 commented May 6, 2020

/it

@stippi2
Copy link
Member Author

stippi2 commented May 6, 2020

/it

@daniel-kurzynski
Copy link
Member

/it

@daniel-kurzynski daniel-kurzynski merged commit 082b249 into master May 6, 2020
@daniel-kurzynski daniel-kurzynski deleted the logrus-buffer-issue branch May 6, 2020 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants