# Build process refactor

* :notebook: https://github.com/readthedocs/readthedocs.org/issues/3984
* :palm_tree: https://github.com/readthedocs/readthedocs.org/pull/8815

## Refactor's decisions

- [x] Remove all non-required/non-used code
- [ ] ~~Do not use `self.` variables anymore. Pass all the variables required to perform the action instead~~
- [x] Use `log.bind` when possible to avoid duplication of keywords
- [ ] ~~Convert context managers into `.initialize()` methods.~~
- I'm not doing this refactor on this PR for now. I'm not sure yet it's a good decision
- [ ] ~~Use only one environment and create/initialize it at beginning (if not possible, create `vcs_environment` and `build_environment`)~~.
- We need to clone the repository to know the Docker image to be used for `build_environment`
- What about using the API to download the contents of `.readthedocs.yaml` and spin up only one container?
- [x] Remove outdated leafovers that are not used
- [x] Move "update build object" to `on_success`, `on_failure` and `after_return`
- [x] Remove `update_on_success` magic from inside the `BuildEnvironment` and update the build status from the normal flow inside the tast
- [x] `BuildEnvironment` should only be in charge of running the commands
- It should not update the build status, handle build exceptions, etc
- `BuildCommand` objects _could_ be updated from outside to make it cleaner (this is not done in this PR)
- [x] Every known error should raise an exception (e.g. YAML issue, command failed, build timeout, etc) to be handled at `on_failure` and decide what to do
- Exceptions are differentiated between `BuildUserError`, `BuildAppError` and unknown exceptions.
- [ ] ~~Use methods per each build state (`clonning`, `installing`, `building`, `uploading`)~~.
- I'm not doing this refactor on this PR. We can come back to it if we finally consider it will improve code readability

### Workflow after aplying these decisions

With these decisions we will have the logic clearly separated into different responsabilities:

- Celery task will perform
1. a set up _before_ running the task to grab all the required data via API (e.g. version, project, build, etc)
1. call `execute` method to run all the commands inside a `BuildEnvironment`
1. handle _all_ the exceptions that may have occurred during the task under the `on_failure` method
1. finalize the task by sending notifications to users depending on the result (`on_success` or `on_failre`)
1. do a cleanup of the current task (`after_return`)
- `BuildEnvironment` will _only_ be in charge of running the commands and saving their results via the API

## Environment caching not supported

We are using `CLEAN_AFTER_BUILD` in production and all this code does not make sense anymore.

- [x] Delete json hash to decide if the environment is obsolete
* Remove `save_environment_json`
* Remove `is_obsolote`
* ... and others related
- [x] Delete Cache saved in storage
- [x] Always use pip with `--no-cache-dir`
- If anything, we should make usage of `devpi` instead (see https://github.com/readthedocs/readthedocs.org/pull/6340)
- [x] Delete "Wipe" feature
- [x] Delete locks (eg. `repo_nonblockinglock`) and `LockTimeout` exception

> We can recover this code if we need it in the future, but there is no reason to maintain it if we are not using it.

## Local environment not supported

We do not support local build environment anymore (build outside Docker and using the host system to perform the build)

- [ ] Delete `LocalBuildEnvironment`
- [ ] Requires updating lot of tests
- [ ] `BuildCommand.run` can be deleted as well (it uses `subprocess.Popen`)

## Problems

### General problems

- Passing build data from Celery task to `DockerBuildEnvironment` and vice-versa is not trivial.
- Celery task _and_ `DockerBuildEnvironment` / `DockerBuildCommand` hit the API to update things related to the build. It would be good if we can avoid/optimize this to hit the API only once at the end.
- Note this is not possible for `BuildCommand` because we want instant update of each command executed
- We use `self` inside our Task. Because of that reason we need to do a clean up of all this data before the task ends. See https://docs.celeryproject.org/en/master/userguide/tasks.html#instantiation
- Since we are using `self` to store data used by methods to be called in the "future", it's hard to know if the order of them can be altered. It would be good if we can avoid this technique as much as possible and only have as few as possible "global" variables (e.g. `self.project`, `self.version`, `self.environment`, etc)
- We are using `APIProject` and `APIVersion`. However, we don't have an `APIBuild` and we are using just a dictionary.
- I wasn't able to create an `APIBuild` when I tried
- I wanted to have consistency here

### Tests' problems

- We have a lot of hacks in our tests (e.g. https://github.com/readthedocs/readthedocs.org/pull/6482#discussion_r367694530) that makes things really hard
- VCSs (git, hg, svn), doctools (`BaseSphinx`, `BaseMkdocs`) and other classes are _tied_ to the environment where they execute the commands but we are heavily hacking them in the tests
- Lot of API calls are not mocked
- We have lot of integration tests requiring a build environment when it shouldn't be needed. They could be done by a unit-test testing the underlying logic isolated
- This is a good example of what I'm saying: https://github.com/readthedocs/readthedocs.org/pull/8815/commits/bc8c4e8104dc16b680eeae4254cf13c63d92659d
- This class/file makes an extensive usage of this hacky technique: https://github.com/readthedocs/readthedocs.org/blob/640b3d303ad60992b7f1d4f6a512eb7a28d4359d/readthedocs/rtd_tests/tests/test_config_integration.py#L338
- Test make extensive use of `LocalBuildEnvironment` which is deprecated and does not work properly --we probably need a `TestBuildEnvironment` instead with more control and for test-only purposes
- We are executing a lot of real `git` commands on tests that should not be needed. All these responses should be mocked if possible
- Many tests use ConfigV1 to check (default) options
- `readthedocs.rtd_tests.mocks.environment.EnvironmentMockGroup` is a **huge hack** and we should probably try to kill it
- Really hard to **mock the git repository**
- first time we need to fail at calling `git.Repo` so it doesn't exist and we perform the normal `git clone` (where `.run` method is mocked)
- after that, we want to have a _real_ repository on disk so `git.Repo` works and we can grab `repository.tags`, etc
- We are currently using `LocalBuildEnvironment`, even with the new mock system. It would be way better if we can use `DockerBuildEnvironment` instead with all the Docker API client mocked.

## Other notes

- Check what were the changes in `readthedocs/projects/tasks.py` between 10/20 commits before the Django3 migration to after it. I found some conflicts and I'm not sure I solved them properly
- `sync_repository_task` is executed by builders. This means, that if we need to clone big repositories to sync versions/branches/tags it will have an impact on builders building documentation
- by using `git ls-remote` we could execute these tasks on `web-celery` instances instead, and free builders to do builds only
- unfortunately, there are many projects where we cannot use `git ls-remote`
- Is there any case were `record=False` make sense on `update_docs_task`?
- it produces not having a `build` object available at build time, which doesn't make sense to me
- see https://github.com/readthedocs/readthedocs.org/blob/5abb5a78871375b4b6f526628e4115cc9fb81b66/readthedocs/core/utils/__init__.py#L82-L91
- we should probably remove the `record=` argument for the task and related ones
- all the same questions apply for `force=False`/`force=True` :thinking_face:
- note that `record=False` makes sense for the command itself, tho. There are cases where we want to execute the command but do not log/show it to the user (e.g. `apt-get install certificates` or `git rev-parse HEAD`)
- Since we suport ConfigV1 and ConfigV2 with different defaults, executing the "normal" flow with one or the other produces a lot of different commands being executed. To be able to them all, we need to write a lot of tests
- If we do _one_ test covering all the properties from the config file and check all the commands, it should be enough as an integration test
- All the other properties can be tested as unit-tests for the Config object itself
- We should test how we select the `container_image` based on feature flag, config file, or `Project.container_image`
- Review `time_limit` from `DOCKER_LIMITS` _and_ for Celery task so we can handle them nicely at `on_timeout`. In the last case, we will be able to do a nice cleanup.
- We should probably remove all the code from VCS backends to "update" the repository. Since we are doing a full clean before/after the build; we want to always do a full clone
- Update build command error report to not show the whole stdout from the command
- ~~Remove celery priority queues since they don't really work~~
- Open an issue to continue with this work https://github.com/readthedocs/readthedocs.org/pull/8815#discussion_r791146335

    # 1. raise Reject(requeue=False) on duplicated builds
    # 2. use a global `task_cls` (https://docs.celeryproject.org/en/latest/userguide/tasks.html#app-wide-usage) to logs actions
    # 3. use CELERY_IMPORTS to register the tasks (https://docs.celeryproject.org/en/latest/userguide/configuration.html#std-setting-imports)
    # 4. use CELERY_TASK_IGNORE_RESULT=True since we are not using the result at all

- Finish this PR https://github.com/readthedocs/readthedocs.org/pull/8031
- Review this https://github.com/readthedocs/readthedocs.org/pull/8269 and check if we can deploy it now

----

----

### How to create a mocked build test

We need to find a way to mock:

- APIv2 calls (`get_version`, `get_project`, concurrency limit, POST command outputs, etc)
- load a valid git repository
- load a valid config file into memory
- do not execute build commands
- load a git repository in memory (e.g. `git.Repo`) by mocking it
- if this is not possible, mock all the logic around the repository

----
----

## TODO on this PR

- [x] Remove `EnvironmentMockGroup`
- [x] Finish translating tests into new system
- [x] Fix task naming to work with new and _old_ instances https://github.com/readthedocs/readthedocs.org/pull/8815#discussion_r791134052
- [x] The signature of the task changed, so old webs cannot trigger `update_docs_task` that the new builder can execute :(

readthedocs[75471]: [warning  ] /home/docs/lib/python3.8/site-packages/celery/app/trace.py:657: RuntimeWarning: Exception raised outside body: AttributeError("'update_docs_task' object has no attribute 'build_pk'"):
Traceback (most recent call last):
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 450, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/home/docs/lib/python3.8/site-packages/newrelic/hooks/application_celery.py", line 99, in wrapper
    return wrapped(*args, **kwargs)
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 731, in __protected_call__
    return self.run(*args, **kwargs)
  File "/home/docs/lib/python3.8/site-packages/sentry_sdk/integrations/celery.py", line 197, in _inner
    reraise(*exc_info)
  File "/home/docs/lib/python3.8/site-packages/sentry_sdk/_compat.py", line 54, in reraise
    raise value
  File "/home/docs/lib/python3.8/site-packages/sentry_sdk/integrations/celery.py", line 192, in _inner
    return f(*args, **kwargs)
  File "/home/docs/lib/python3.8/site-packages/celery/app/autoretry.py", line 34, in run
    return task._orig_run(*args, **kwargs)
TypeError: update_docs_task() got an unexpected keyword argument 'record'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 467, in trace_task
    I, R, state, retval = on_error(task_request, exc, uuid)
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 381, in on_error
    R = I.handle_error_state(
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 178, in handle_error_state
    return {
  File "/home/docs/lib/python3.8/site-packages/celery/app/trace.py", line 231, in handle_failure
    task.on_failure(exc, req.id, req.args, req.kwargs, einfo)
  File "/home/docs/checkouts/readthedocs.org/readthedocs/projects/tasks/builds.py", line 342, in on_failure
    'id': self.build_pk,
AttributeError: 'update_docs_task' object has no attribute 'build_pk'

- [ ] Investigate more the usage of `self` for the task itself ()
- https://github.com/readthedocs/readthedocs.org/pull/8815#discussion_r792039515
- [x] Pass the required arguments to the task https://github.com/readthedocs/readthedocs.org/pull/8815#discussion_r791158450

### Attempt to not use `self.` inside the task

* :label: https://github.com/readthedocs/readthedocs.org/pull/8815#discussion_r792039515