moby / term

Apache License 2.0
66 stars 27 forks source link

Remove debug logs and logrus dependency, and go mod tidy #5

Closed thaJeztah closed 4 years ago

thaJeztah commented 4 years ago

Logrus was only used for debug logging

~This patch takes a similar approach as was done in the upstream github.com/Azure/go-ansiterm repository (https://github.com/Azure/go-ansiterm/pull/25).~

thaJeztah commented 4 years ago

@cpuguy83 @dims @kolyshkin PTAL

(FWIW; I'm inclined to remove these logs altogether as this package wasn't touched for years, so probably the most apparent bugs should be fixed and no debugging logs needed, unless we're concerned about future Windows updates changing behavior)

dims commented 4 years ago

LGTM, i like the idea of removing the logs altogether

thaJeztah commented 4 years ago

Yes, most of these logs were added when Windows support for containers was still in very early stages, so debugging and failures was needed. Perhaps it's no longer relevant at all. I did see the Azure repository also using it, so perhaps there's still internal use of this inside Microsoft ?

Also looking at some other functions in this package that could possibly be replaced by functionality from Go's standard library (or golang.org/x/sys)

cpuguy83 commented 4 years ago

I don't really see this change as better.

thaJeztah commented 4 years ago

@cpuguy83 it removes a dependency (I think Ideally a library package should have no, or minimal dependencies). However, as mentioned above, I'm included to remove these logs altogether: would that be more what you're looking for?

cpuguy83 commented 4 years ago

Yes, I think that's best.

On Thu, Apr 23, 2020 at 1:48 PM Sebastiaan van Stijn < notifications@github.com> wrote:

@cpuguy83 https://github.com/cpuguy83 it removes a dependency (I think Ideally a library package should have no, or minimal dependencies). However, as mentioned above, I'm included to remove these logs altogether: would that be more what you're looking for?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/moby/term/pull/5#issuecomment-618661172, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGDCZTGUK3J2CEGWRWYCQDROCSTVANCNFSM4MN7QGJA .

--

thaJeztah commented 4 years ago

@cpuguy83 updated; I removed the logs

dims commented 4 years ago

LGTM