Andrushika commented on code in PR #75001: URL: https://github.com/apache/airflow/pull/75001#discussion_r4336290248
########## scripts/ci/docker-compose/mount-sphinx-airflow-theme.yml: ########## @@ -0,0 +1,26 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +--- +services: + airflow: + # Added by `breeze build-docs --sphinx-airflow-theme-path`. The mount is read-only so the + # in-container install cannot leave root-owned build output in the airflow-site checkout. + volumes: + - type: bind + source: ${SPHINX_AIRFLOW_THEME_PATH} + target: /opt/sphinx_airflow_theme + read_only: true Review Comment: I think the read-only mount is fine, since the theme builds with `flit_core` and does not write into the source tree. I ran the install against a read-only mount in the CI image and it worked. ########## scripts/in_container/run_docs_build.sh: ########## @@ -21,6 +21,12 @@ git config --global --add safe.directory /opt/airflow cd "${AIRFLOW_SOURCES}" || exit 1 + +# Mounted by `breeze build-docs --sphinx-airflow-theme-path`. +if [[ -d /opt/sphinx_airflow_theme ]]; then + uv pip install --no-deps --reinstall /opt/sphinx_airflow_theme Review Comment: I think `--no-deps` is right here, because with `--reinstall` uv would otherwise reinstall all of Sphinx's dependencies. If theme deps are needed, `--reinstall-package sphinx-airflow-theme` without `--no-deps` only touches the theme. ########## dev/breeze/tests/test_developer_commands.py: ########## @@ -109,6 +109,53 @@ def test_python_option_is_rejected(self, runner): assert "no such option" in result.output.lower() +class TestBuildDocsSphinxAirflowTheme: + @pytest.fixture(autouse=True) + def _isolated_env(self, monkeypatch, tmp_path): + monkeypatch.setenv("SKIP_SAVING_CHOICES", "true") + for name in ("BREEZE_SDK", "DISTRIBUTIONS_LIST"): + monkeypatch.delenv(name, raising=False) + # The real _build_python_docs deletes generated/**/apis under AIRFLOW_ROOT_PATH. + monkeypatch.setattr("airflow_breeze.commands.developer_commands.AIRFLOW_ROOT_PATH", tmp_path) + + @pytest.fixture + def docker_mocks(self): + with patch.multiple( + "airflow_breeze.commands.developer_commands", + perform_environment_checks=DEFAULT, + fix_ownership_using_docker=DEFAULT, + cleanup_python_generated_files=DEFAULT, + build_ci_image_if_needed=DEFAULT, + execute_command_in_shell=DEFAULT, + autospec=True, + ) as mocks: + mocks["execute_command_in_shell"].return_value.returncode = 0 + yield mocks + + @pytest.mark.parametrize("pass_option", [True, False]) + def test_theme_path_is_passed_to_shell_params( + self, runner, docker_mocks, tmp_path, monkeypatch, pass_option + ): + (tmp_path / "theme" / "sphinx_airflow_theme" / "static" / "_gen").mkdir(parents=True) + monkeypatch.chdir(tmp_path) + + runner.invoke( + build_docs, + ["--sphinx-airflow-theme-path", "theme"] if pass_option else [], + catch_exceptions=False, + ) + + shell_params = docker_mocks["execute_command_in_shell"].call_args.args[0] + assert shell_params.sphinx_airflow_theme_path == (str(tmp_path / "theme") if pass_option else "") + + def test_theme_without_generated_assets_is_rejected(self, runner, docker_mocks, tmp_path): Review Comment: We might need `(tmp_path / "sphinx_airflow_theme").mkdir()` here. With an empty folder, the test also passes when the check only looks for the package folder. ########## dev/breeze/src/airflow_breeze/commands/developer_commands.py: ########## @@ -1052,11 +1060,21 @@ def build_docs( distributions_list: str, sdk: tuple[str, ...], sdk_docs_only: bool, + sphinx_airflow_theme_path: str | None, doc_packages: tuple[str, ...], ): """ Build documents. """ + if ( Review Comment: Nit: rejecting `--sdk-docs-only` with this option looks simpler, since the Java and TypeScript SDK docs don't use the Sphinx theme. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
