Open
Description
What's the problem this feature will solve?
The handling of execution of commands is done at https://github.com/readthedocs/readthedocs.org/blob/main/readthedocs/doc_builder/environments.py.
That file has two levels of indirections (BuildCommand and BuildEnvironment), and also two subclasses (Docker related execution and local related execution), and both are really coupled (BuildCommand depends on BuildEnvironment, instead of BuildEnvironment depend on BuildCommand).
This leads to the Docker related execution code to not be tested at all, and makes it hard to work with (like passing just one extra env var to a command), we also have problems with escaping (#12152, #9397).
Describe the solution you'd like
- Have two classes, BuildEnvironment and BuildCommand, but make BuildCommand not depend on BuildEnvironment, as its only job should be to run the actual commands, don't think we need two subclasses of BuildEnvironment.
- Make this code easier to mock so we write proper tests for it, and only actually run commands for tests that actually need to do so.
- Allow executing arbitrary commands without having to re-instantiate the class.
- Allow to pass extra things to commands after the main class has been instantiated (like env vars per commands!)
- Use https://docs.python.org/3/library/shlex.html#shlex.join instead of implementing our own escaping logic.
I'm just sketching some general ideas, haven't look deep into the code about how this will actually look like.