From 80c314067d5319ccc5605700f2ecf208ae02ebd0 Mon Sep 17 00:00:00 2001 From: Benjamin Bach Date: Fri, 10 Mar 2023 18:50:09 +0100 Subject: [PATCH] Updates: Rename new field to build_config_file, add on Build and Version data, relax user input validation, introduce very basic reproducibility of builds --- readthedocs/api/v2/serializers.py | 2 +- readthedocs/builds/admin.py | 1 + .../migrations/0048_add_build_config_file.py | 35 +++++++++++++ readthedocs/builds/models.py | 17 ++++++- readthedocs/config/config.py | 49 ++++++++++++------- readthedocs/doc_builder/backends/mkdocs.py | 2 + readthedocs/doc_builder/config.py | 13 +++-- readthedocs/doc_builder/director.py | 37 ++++++++++++-- readthedocs/projects/forms.py | 24 ++++----- .../migrations/0096_auto_20230207_1642.py | 35 ------------- .../migrations/0099_add_build_config_file.py | 37 ++++++++++++++ readthedocs/projects/models.py | 21 ++++---- readthedocs/rtd_tests/tests/test_api.py | 13 +++-- .../tests/test_config_integration.py | 2 +- 14 files changed, 197 insertions(+), 91 deletions(-) create mode 100644 readthedocs/builds/migrations/0048_add_build_config_file.py delete mode 100644 readthedocs/projects/migrations/0096_auto_20230207_1642.py create mode 100644 readthedocs/projects/migrations/0099_add_build_config_file.py diff --git a/readthedocs/api/v2/serializers.py b/readthedocs/api/v2/serializers.py index 36eaa2e0b00..faa413796dc 100644 --- a/readthedocs/api/v2/serializers.py +++ b/readthedocs/api/v2/serializers.py @@ -92,7 +92,7 @@ class Meta(ProjectSerializer.Meta): "show_advertising", "environment_variables", "max_concurrent_builds", - "rtd_conf_file", + "build_config_file", ) diff --git a/readthedocs/builds/admin.py b/readthedocs/builds/admin.py index 83602b49a2d..9fb0ebc32aa 100644 --- a/readthedocs/builds/admin.py +++ b/readthedocs/builds/admin.py @@ -33,6 +33,7 @@ class BuildAdmin(admin.ModelAdmin): "date", "builder", "length", + "build_config_file", "pretty_config", ) readonly_fields = ( diff --git a/readthedocs/builds/migrations/0048_add_build_config_file.py b/readthedocs/builds/migrations/0048_add_build_config_file.py new file mode 100644 index 00000000000..24286188b40 --- /dev/null +++ b/readthedocs/builds/migrations/0048_add_build_config_file.py @@ -0,0 +1,35 @@ +# Generated by Django 3.2.18 on 2023-03-10 16:20 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("builds", "0047_build_default_triggered"), + ] + + operations = [ + migrations.AddField( + model_name="build", + name="build_config_file", + field=models.CharField( + blank=True, + default=None, + max_length=1024, + null=True, + verbose_name="Non-default build configuration file used", + ), + ), + migrations.AddField( + model_name="version", + name="build_config_file", + field=models.CharField( + blank=True, + default=None, + max_length=1024, + null=True, + verbose_name="Non-default build configuration file used", + ), + ), + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index fafc68cecaa..89019bf7985 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -65,11 +65,9 @@ BITBUCKET_COMMIT_URL, BITBUCKET_URL, DOCTYPE_CHOICES, - GITHUB_BRAND, GITHUB_COMMIT_URL, GITHUB_PULL_REQUEST_COMMIT_URL, GITHUB_URL, - GITLAB_BRAND, GITLAB_COMMIT_URL, GITLAB_MERGE_REQUEST_COMMIT_URL, GITLAB_URL, @@ -192,6 +190,14 @@ class Version(TimeStampedModel): # Only include EXTERNAL type Versions. external = ExternalVersionManager.from_queryset(partial(VersionQuerySet, external_only=True))() + build_config_file = models.CharField( + _("Non-default build configuration file used"), + max_length=1024, + default=None, + blank=True, + null=True, + ) + class Meta: unique_together = [('project', 'slug')] ordering = ['-verbose_name'] @@ -702,6 +708,13 @@ class Build(models.Model): null=True, blank=True, ) + build_config_file = models.CharField( + _("Non-default build configuration file used"), + max_length=1024, + default=None, + blank=True, + null=True, + ) length = models.IntegerField(_('Build Length'), null=True, blank=True) diff --git a/readthedocs/config/config.py b/readthedocs/config/config.py index c6995b3e369..3175fdffc9c 100644 --- a/readthedocs/config/config.py +++ b/readthedocs/config/config.py @@ -43,17 +43,18 @@ ) __all__ = ( - 'ALL', - 'load', - 'BuildConfigV1', - 'BuildConfigV2', - 'ConfigError', - 'ConfigOptionNotSupportedError', - 'ConfigFileNotFound', - 'InvalidConfig', - 'PIP', - 'SETUPTOOLS', - 'LATEST_CONFIGURATION_VERSION', + "ALL", + "load", + "BuildConfigV1", + "BuildConfigV2", + "ConfigError", + "ConfigOptionNotSupportedError", + "ConfigFileNotFound", + "DefaultConfigFileNotFound", + "InvalidConfig", + "PIP", + "SETUPTOOLS", + "LATEST_CONFIGURATION_VERSION", ) ALL = 'all' @@ -96,6 +97,17 @@ def __init__(self, directory): ) +class DefaultConfigFileNotFound(ConfigError): + + """Error when we can't find a configuration file.""" + + def __init__(self, directory): + super().__init__( + f"No default configuration file in: {directory}", + CONFIG_FILE_REQUIRED, + ) + + class ConfigOptionNotSupportedError(ConfigError): """Error for unsupported configuration options in a version.""" @@ -1377,17 +1389,18 @@ def load(path, env_config, config_file=None): the version of the configuration a build object would be load and validated, ``BuildConfigV1`` is the default. """ - if config_file is None or config_file == "": + if not config_file: filename = find_one(path, CONFIG_FILENAME_REGEX) + if not filename: + # This exception is current caught higher up and will result in an attempt + # to load the v1 config schema. + raise DefaultConfigFileNotFound(os.path.relpath(filename, path)) else: filename = os.path.join(path, config_file) + # When a config file is specified and not found, we raise ConfigError + # because ConfigFileNotFound if not os.path.exists(filename): - raise ConfigError( - f".readthedocs.yml not found at {config_file}", CONFIG_FILE_REQUIRED - ) - - if not filename: - raise ConfigFileNotFound(path) + raise ConfigFileNotFound(os.path.relpath(filename, path)) # Allow symlinks, but only the ones that resolve inside the base directory. with safe_open( diff --git a/readthedocs/doc_builder/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 0d0174176d0..2831d456ee5 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -45,6 +45,8 @@ class BaseMkdocs(BaseBuilder): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + + # This is the *MkDocs* yaml file self.yaml_file = self.get_yaml_config() # README: historically, the default theme was ``readthedocs`` but in diff --git a/readthedocs/doc_builder/config.py b/readthedocs/doc_builder/config.py index 876b0cb7ba3..6b5b7cc1fc8 100644 --- a/readthedocs/doc_builder/config.py +++ b/readthedocs/doc_builder/config.py @@ -2,19 +2,22 @@ from os import path -from readthedocs.config import BuildConfigV1, ConfigFileNotFound +from readthedocs.config import BuildConfigV1 from readthedocs.config import load as load_config from readthedocs.projects.models import ProjectConfigurationError +from ..config.config import DefaultConfigFileNotFound from .constants import DOCKER_IMAGE, DOCKER_IMAGE_SETTINGS -def load_yaml_config(version): +def load_yaml_config(version, config_file=None): """ - Load a configuration from `readthedocs.yml` file. + Load a build configuration file. This uses the configuration logic from `readthedocs-build`, which will keep parsing consistent between projects. + + :param config_file: Optionally, we are told which config_file to load instead of using defaults. """ checkout_path = version.project.checkout_path(version.slug) project = version.project @@ -56,9 +59,9 @@ def load_yaml_config(version): config = load_config( path=checkout_path, env_config=env_config, - config_file=project.rtd_conf_file, + config_file=config_file, ) - except ConfigFileNotFound: + except DefaultConfigFileNotFound: # Default to use v1 with some defaults from the web interface # if we don't find a configuration file. config = BuildConfigV1( diff --git a/readthedocs/doc_builder/director.py b/readthedocs/doc_builder/director.py index c7aecd62910..b292fd98200 100644 --- a/readthedocs/doc_builder/director.py +++ b/readthedocs/doc_builder/director.py @@ -1,3 +1,12 @@ +""" +The ``director`` module can be seen as the entrypoint of the build process. + +It "directs" all of the high-level build jobs: + +* checking out the repo +* setting up the environment +* fetching instructions etc. +""" import os import tarfile @@ -194,17 +203,36 @@ def build(self): # VCS checkout def checkout(self): - log.info( - "Clonning repository.", - ) + """Checkout Git repo and load build config file.""" + + log.info("Cloning repository.") self.vcs_repository.update() identifier = self.data.build_commit or self.data.version.identifier log.info("Checking out.", identifier=identifier) self.vcs_repository.checkout(identifier) - self.data.config = load_yaml_config(version=self.data.version) + # The director is responsible for understanding which config file to use for a build. + # Reproducibility: Build already has a config_file set. + non_default_config_file = self.data.build.get("build_config_file", None) + + # This logic can be extended with version-specific config files + if not non_default_config_file and self.data.version.project.build_config_file: + non_default_config_file = self.data.version.project.build_config_file + + if non_default_config_file: + log.info(f"Using a non-default config file {non_default_config_file}.") + self.data.config = load_yaml_config( + version=self.data.version, + config_file=non_default_config_file, + ) self.data.build["config"] = self.data.config.as_dict() + self.data.build["build_config_file"] = non_default_config_file + + # Config file was successfully loaded and a custom path was used + if non_default_config_file and not self.data.version.build_config_file: + self.data.version.build_config_file = non_default_config_file + # TODO: Do we call self.data.version.save() here? if self.vcs_repository.supports_submodules: self.vcs_repository.update_submodules(self.data.config) @@ -357,6 +385,7 @@ def check_old_output_directory(self): raise BuildUserError(BuildUserError.BUILD_OUTPUT_OLD_DIRECTORY_USED) def run_build_commands(self): + """Runs each build command in the build environment.""" reshim_commands = ( {"pip", "install"}, {"conda", "create"}, diff --git a/readthedocs/projects/forms.py b/readthedocs/projects/forms.py index 8b2cef3a627..72e521b885c 100644 --- a/readthedocs/projects/forms.py +++ b/readthedocs/projects/forms.py @@ -207,7 +207,7 @@ class Meta: "single_version", "external_builds_enabled", "external_builds_privacy_level", - "rtd_conf_file", + "build_config_file", ) # These that can be set per-version using a config file. per_version_settings = ( @@ -358,15 +358,8 @@ def clean_conf_py_file(self): ) # yapf: disable return filename - def clean_rtd_conf_file(self): - filename = self.cleaned_data.get("rtd_conf_file", "").strip() - if filename and ".readthedocs.yml" not in filename: - raise forms.ValidationError( - _( - "Your configuration file is invalid, make sure it contains " - ".readthedocs.yml in it.", - ), - ) # yapf: disable + def clean_build_config_file(self): + filename = self.cleaned_data.get("build_config_file", "").strip() return filename def get_all_active_versions(self): @@ -391,7 +384,9 @@ class UpdateProjectForm( ProjectExtraForm, ): - class Meta: + """Basic project settings form for Admin.""" + + class Meta: # noqa model = Project fields = ( # Basics @@ -407,6 +402,7 @@ class Meta: ) def clean_language(self): + """Ensure that language isn't already active.""" language = self.cleaned_data['language'] project = self.instance if project: @@ -540,6 +536,8 @@ def save(self): class WebHookForm(forms.ModelForm): + """Webhook form.""" + project = forms.CharField(widget=forms.HiddenInput(), required=False) class Meta: @@ -607,6 +605,8 @@ def get_choices(self): ) for project in self.get_translation_queryset().all()] def clean_project(self): + """Ensures that selected project is valid as a translation.""" + translation_project_slug = self.cleaned_data['project'] # Ensure parent project isn't already itself a translation @@ -726,6 +726,7 @@ def clean_project(self): return self.project def clean_domain(self): + """Validates domain.""" domain = self.cleaned_data['domain'].lower() parsed = urlparse(domain) @@ -864,6 +865,7 @@ def clean_project(self): return self.project def clean_name(self): + """Validate environment variable name chosen.""" name = self.cleaned_data['name'] if name.startswith('__'): raise forms.ValidationError( diff --git a/readthedocs/projects/migrations/0096_auto_20230207_1642.py b/readthedocs/projects/migrations/0096_auto_20230207_1642.py deleted file mode 100644 index f16e908ac89..00000000000 --- a/readthedocs/projects/migrations/0096_auto_20230207_1642.py +++ /dev/null @@ -1,35 +0,0 @@ -# Generated by Django 3.2.17 on 2023-02-07 16:42 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ("projects", "0095_default_branch_helptext"), - ] - - operations = [ - migrations.AddField( - model_name="historicalproject", - name="rtd_conf_file", - field=models.CharField( - blank=True, - default="", - help_text="Path from project root to .readthedocs.yml file (ex. docs/.readthedocs.yml). Leave blank if you want us to find it for you.", - max_length=255, - verbose_name=".readthedocs.yml configuration file", - ), - ), - migrations.AddField( - model_name="project", - name="rtd_conf_file", - field=models.CharField( - blank=True, - default="", - help_text="Path from project root to .readthedocs.yml file (ex. docs/.readthedocs.yml). Leave blank if you want us to find it for you.", - max_length=255, - verbose_name=".readthedocs.yml configuration file", - ), - ), - ] diff --git a/readthedocs/projects/migrations/0099_add_build_config_file.py b/readthedocs/projects/migrations/0099_add_build_config_file.py new file mode 100644 index 00000000000..dce4a5810db --- /dev/null +++ b/readthedocs/projects/migrations/0099_add_build_config_file.py @@ -0,0 +1,37 @@ +# Generated by Django 3.2.18 on 2023-03-10 16:20 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("projects", "0098_pdf_epub_opt_in"), + ] + + operations = [ + migrations.AddField( + model_name="historicalproject", + name="build_config_file", + field=models.CharField( + blank=True, + default=None, + help_text="Warning: experimental feature. Custom path from repository root to a build configuration file, ex. subproject/docs/.readthedocs.yaml. Leave blank for default value (.readthedocs.yaml).", + max_length=1024, + null=True, + verbose_name="Build configuration file", + ), + ), + migrations.AddField( + model_name="project", + name="build_config_file", + field=models.CharField( + blank=True, + default=None, + help_text="Warning: experimental feature. Custom path from repository root to a build configuration file, ex. subproject/docs/.readthedocs.yaml. Leave blank for default value (.readthedocs.yaml).", + max_length=1024, + null=True, + verbose_name="Build configuration file", + ), + ), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 256f07d8408..bce3b84bded 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -350,7 +350,7 @@ class Project(models.Model): conf_py_file = models.CharField( _('Python configuration file'), max_length=255, - default='', + default="", blank=True, help_text=_( "Path from project root to conf.py file " @@ -358,15 +358,18 @@ class Project(models.Model): "Leave blank if you want us to find it for you.", ), ) - rtd_conf_file = models.CharField( - _(".readthedocs.yml configuration file"), - max_length=255, - default="", + + build_config_file = models.CharField( + _("Build configuration file"), + max_length=1024, + default=None, blank=True, + null=True, help_text=_( - "Path from project root to .readthedocs.yml file " - "(ex. docs/.readthedocs.yml). " - "Leave blank if you want us to find it for you.", + "Warning: experimental feature. " + "Custom path from repository root to a build configuration file, " + "ex. subproject/docs/.readthedocs.yaml. " + "Leave blank for default value (.readthedocs.yaml).", ), ) @@ -865,7 +868,7 @@ def artifact_path(self, type_, version=LATEST): return os.path.join(self.checkout_path(version=version), "_readthedocs", type_) def conf_file(self, version=LATEST): - """Find a ``conf.py`` file in the project checkout.""" + """Find a Sphinx ``conf.py`` file in the project checkout.""" if self.conf_py_file: conf_path = os.path.join( self.checkout_path(version), diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index a511bbcc74a..64bd30257cb 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -705,7 +705,10 @@ def test_user_doesnt_get_full_api_return(self): user_normal = get(User, is_staff=False) user_admin = get(User, is_staff=True) project = get( - Project, main_language_project=None, conf_py_file="foo", rtd_conf_file="bar" + Project, + main_language_project=None, + conf_py_file="foo", + build_config_file="bar", ) client = APIClient() @@ -713,15 +716,15 @@ def test_user_doesnt_get_full_api_return(self): resp = client.get('/api/v2/project/%s/' % (project.pk)) self.assertEqual(resp.status_code, 200) self.assertNotIn("conf_py_file", resp.data) - self.assertNotIn("rtd_conf_file", resp.data) + self.assertNotIn("build_config_file", resp.data) client.force_authenticate(user=user_admin) resp = client.get('/api/v2/project/%s/' % (project.pk)) self.assertEqual(resp.status_code, 200) self.assertIn("conf_py_file", resp.data) self.assertEqual(resp.data["conf_py_file"], "foo") - self.assertIn("rtd_conf_file", resp.data) - self.assertEqual(resp.data["rtd_conf_file"], "bar") + self.assertIn("build_config_file", resp.data) + self.assertEqual(resp.data["build_config_file"], "bar") def test_project_features(self): user = get(User, is_staff=True) @@ -2476,7 +2479,7 @@ def test_get_version_by_id(self): "repo": "https://github.com/pypa/pip", "repo_type": "git", "requirements_file": None, - "rtd_conf_file": "", + "build_config_file": None, "show_advertising": True, "skip": False, "slug": "pip", diff --git a/readthedocs/rtd_tests/tests/test_config_integration.py b/readthedocs/rtd_tests/tests/test_config_integration.py index f01876dd92a..e43163d793b 100644 --- a/readthedocs/rtd_tests/tests/test_config_integration.py +++ b/readthedocs/rtd_tests/tests/test_config_integration.py @@ -99,7 +99,7 @@ def test_python_supported_versions_default_image_1_0(self, load_config): load_config.assert_called_once_with( path=mock.ANY, env_config=expected_env_config, - config_file="", + config_file=None, ) self.assertEqual(config.python.version, '3')