Make Docker.run return its stdout as a string

[kit-ty-kate]

After commenting on #99 (comment) I realized it was relatively easy to change Current_docker.run to do what i want and return its result as a string.

This is the current design I'm using in opam-ci where it successfully gets the list of reverse dependencies for each packages:

[talex5]

This looks good, but I think it should be a new function (e.g. Docker.pread), as we often don't need to use (or store) the output.

[craigfe]

Thanks 🙂 Some thoughts:

• the use of Current.Process.check_output rather than docker logs sacrifices the ability to tail the logs, which is perhaps significant for very containers with very verbose output. I'm not sure how docker logs --tail is implemented, but it seems likely to be better than loading the entire log as a string and then truncating it.

• Current.Process.check_output seems to redirect the stdout stream to a string rather than the Current logs, which is problematic as it means those logs are no longer visible in the UI.

• Current.Process.check_output does not capture the stderr stream, unlike the previous attempt at a docker logs solution. This seems like a considerable limitation.

• @talex5: I think we do often need to use the output, but it happens that the Log_matcher is sufficient for this purpose right now. Once we're reporting more rich data back to the user (c.f. GitHub Checks support), it seems that most (all?) of our Docker.run's (at least in the OCaml-CI pipeline) will want to use this feature.

  Which is not to say that we shouldn't still make it opt-in for performance reasons.

edit: I accidentally posted it before finishing... 🤦‍♂️

[talex5]

@craigfe I think this use is quite separate to log analysis. For example, Kate wants to read structured data (revdeps) from the command. This should not be mixed in with warning messages from stderr. It's also quite likely that the output being captured would be binary data, in which case it's important that we don't log it.

[craigfe]

@talex5 : The use-cases I can think of would all want the data to be logged as well as captured. This includes the application to benchmarking (cc @gs0510, @MagnusS), who have been waiting for a feature like this for a while. Is it an issue if the rev-deps, in this case, are not logged to the interface? I think as a new user of OCurrent I would expect them to be.

If we have a use-case for extracting binary data from a Docker container, I suggest doing that by some other mechanism than overloading 'logs'. For instance by using docker cp (as was attempted in the other PR), or volume mounting.

[talex5]

LGTM, once the id is changed.

[talex5]

(we could probably get rid of this per-module thing now - it was a hack for before 4.08, which we no longer support)

[kit-ty-kate]

Should I change this in this PR or in a separate PR?

[kit-ty-kate]

If the change in plugins/docker/dune can be done separately, this should be good to go.

[talex5]

Thanks - I'll take a look at cleaning up the preprocessing separately...

[craigfe]

I don't think this should have been merged.

• The logging behaviour may be problematic for our other use-case, and at the very least seems to warrant further discussion.
• plugins/docker/pread.ml duplicates all of the code from plugins/docker/run.ml, when this seems unnecessary (they share the same Key module, for instance).
• It's missing a test in test/plugins/docker/, ideally capturing the behaviours w.r.t. logging and stderr.
• The docstring for pread doesn't explain that the logging behaviour is not the same as run.

Are we in a rush to merge this feature as is?

[talex5]

The behaviour of pread (returning the output instead of sending it to stdout) is pretty standard, e.g. compare lwt's version:

utop # #require "lwt.unix";;
utop # open Lwt_process;;
utop # let cmd = "", [| "echo"; "hello" |];;

utop # exec cmd;;
hello
- : Unix.process_status = Unix.WEXITED 0

utop # pread cmd;;
- : string = "hello\n"

But I do think we should make it easier to make user-defined pipeline stages using Docker, so the built-in ones don't have to cover every case.

(if you want to add an option to log the output too, or clean up the code, feel free)