LGTM Ack

@firstyear This should actually be handled (at least) in two other locations -- just search for STDOUT: in the code. Could we maybe better make a handler function for this and handle it elsewhere, too?

@mhonek I'm not sure what you have in mind as a handler for this. Could you expand some more?

I think at the end of the day, we'll always hit that there is a difference between bytes / utf8 in python3 now, and we have to be disciplined to do the needed conversions. I can't really think what a "handler" would look like that isn't fixing python 3 to not go bang when it sees bytes ...

In a way we already have handlers, we just have to keep remembering to add them. Alternately we writre wrappers to subprocess .... and then we have to remember to use those tooo ... no matter what, python wil always burden us with having to handle this I think.

@firstyear By the handler function I meant whatever Do-not-Repeat-Yourself solution used in the three places, aka no copy-paste. Like, log.debug(foo(result)) and implement the foo.

Okay, I've fixed the other locations.

The issue is that here the "do not repeat yourself" solution, involves wrapping subprocess and basically making a wrapper to it that can do the log debug when needed, and it's still "up to us" to make sure we use it correctly and all of that fun. There would have to be multiple wrappers for things like run, check_output etc etc ....

All in all, it's a lot of work and effort, and still comes back to "we need to not make mistakes". And that's truly the curse of python, it's always up to us to make sure we do it right.

I think unless this issue becomes really pervasive, that it's not worth the effort. For now, we have a solution in the 3 places that need it. If it happens "more" then we can make the subprocess handlers.

All I wanted to have is a function that takes care of formatting of the log message itself based on the subprocess.CompletedProcess instance, not an almighty subprocess wrapper. But am not going to fight. LGTM.

So is there anything else or is this okay to merge?

Sorry, sure, please go on.

Need to get some upstream builds done today, merging for @firstyear

Thanks @mreynolds I'm at an event so was a bit slow. Appreciate it :)

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3855

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.