Skip to content

Commit

Permalink
Build: fail builds without configuration file or using v1 (#10355)
Browse files Browse the repository at this point in the history
* Config: deprecated notification for builds without config file

When we detect a build is built without a Read the Docs configuration
file (`.readthedocs.yaml`) we show multiple notifications:

- a static warning message in the build detail's page
- a persistent on-site notification to all maintainers/admin of the project
- send a weekly email (at most)

This is the initial step to attempt making users to migrate to our config file
v2, giving them a enough window to do this and avoid breaking their builds in
the future.

Closes #10348

* Test: invert logic

* Build: fail builds without configuration file or using v1

Use a feature flag to decide whether or not hard fail the builds that are not
using a configuration file at all or are using v1.

This will be useful in the future when we want to make the builds to fail during
a reduced period of time to inform users/customers about this deprecation.

Related #10351

* Update copy to follow the same style

* Upgrade `common/` to use the latest versions for pre-commit

This is an attempt to solve the issue with the CircleCI `checks` test.

* Dont trigger the task on each build, and linting

* Linting

* Lint

* Latest `common/`

* Apply suggestions from code review

Co-authored-by: Anthony <[email protected]>

---------

Co-authored-by: Anthony <[email protected]>
  • Loading branch information
humitos and agjohnson authored Jun 14, 2023
1 parent e8f4887 commit bfeb6d8
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 13 deletions.
2 changes: 1 addition & 1 deletion common
6 changes: 6 additions & 0 deletions readthedocs/doc_builder/director.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,12 @@ def checkout(self):
self.data.build["config"] = self.data.config.as_dict()
self.data.build["readthedocs_yaml_path"] = custom_config_file

# Raise a build error if the project is not using a config file or using v1
if self.data.project.has_feature(
Feature.NO_CONFIG_FILE_DEPRECATED
) and self.data.config.version not in ("2", 2):
raise BuildUserError(BuildUserError.NO_CONFIG_FILE_DEPRECATED)

if self.vcs_repository.supports_submodules:
self.vcs_repository.update_submodules(self.data.config)

Expand Down
5 changes: 5 additions & 0 deletions readthedocs/doc_builder/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ class BuildUserError(BuildBaseException):
"Ensure your project is configured to use the output path "
"'$READTHEDOCS_OUTPUT/html' instead."
)
NO_CONFIG_FILE_DEPRECATED = gettext_noop(
"The configuration file required to build documentation is missing from your project. "
"Add a configuration file to your project to make it build successfully. "
"Read more at https://docs.readthedocs.io/en/stable/config-file/v2.html"
)


class BuildUserSkip(BuildUserError):
Expand Down
21 changes: 14 additions & 7 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class ProjectRelationship(models.Model):
def __str__(self):
return '{} -> {}'.format(self.parent, self.child)

def save(self, *args, **kwargs): # pylint: disable=arguments-differ
def save(self, *args, **kwargs):
if not self.alias:
self.alias = self.child.slug
super().save(*args, **kwargs)
Expand Down Expand Up @@ -544,12 +544,14 @@ class Meta:
def __str__(self):
return self.name

def save(self, *args, **kwargs): # pylint: disable=arguments-differ
def save(self, *args, **kwargs):
if not self.slug:
# Subdomains can't have underscores in them.
self.slug = slugify(self.name)
if not self.slug:
raise Exception(_('Model must have slug'))
raise Exception( # pylint: disable=broad-exception-raised
_("Model must have slug")
)
super().save(*args, **kwargs)

try:
Expand All @@ -567,7 +569,7 @@ def save(self, *args, **kwargs): # pylint: disable=arguments-differ
)
self.versions.filter(slug=LATEST).update(identifier=self.default_branch)

def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
def delete(self, *args, **kwargs):
from readthedocs.projects.tasks.utils import clean_project_resources

# Remove extra resources
Expand Down Expand Up @@ -1747,7 +1749,7 @@ class Domain(TimeStampedModel):
canonical = models.BooleanField(
default=False,
help_text=_(
"This domain is the primary one where the documentation is " "served from",
"This domain is the primary one where the documentation is served from",
),
)
https = models.BooleanField(
Expand Down Expand Up @@ -1830,7 +1832,7 @@ def restart_validation_process(self):
self.validation_process_start = timezone.now()
self.save()

def save(self, *args, **kwargs): # pylint: disable=arguments-differ
def save(self, *args, **kwargs):
parsed = urlparse(self.domain)
if parsed.scheme or parsed.netloc:
self.domain = parsed.netloc
Expand Down Expand Up @@ -1949,6 +1951,7 @@ def add_features(sender, **kwargs):
DONT_CREATE_INDEX = "dont_create_index"
USE_RCLONE = "use_rclone"
HOSTING_INTEGRATIONS = "hosting_integrations"
NO_CONFIG_FILE_DEPRECATED = "no_config_file"

FEATURES = (
(ALLOW_DEPRECATED_WEBHOOKS, _("Webhook: Allow deprecated webhook views")),
Expand Down Expand Up @@ -2139,6 +2142,10 @@ def add_features(sender, **kwargs):
"Proxito: Inject 'readthedocs-client.js' as <script> HTML tag in responses."
),
),
(
NO_CONFIG_FILE_DEPRECATED,
_("Build: Building without a configuration file is deprecated."),
),
)

FEATURES = sorted(FEATURES, key=lambda l: l[1])
Expand Down Expand Up @@ -2210,6 +2217,6 @@ class EnvironmentVariable(TimeStampedModel, models.Model):
def __str__(self):
return self.name

def save(self, *args, **kwargs): # pylint: disable=arguments-differ
def save(self, *args, **kwargs):
self.value = quote(self.value)
return super().save(*args, **kwargs)
9 changes: 4 additions & 5 deletions readthedocs/projects/tasks/builds.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ class TaskData:
api_client: API = None

start_time: timezone.datetime = None
# pylint: disable=unsubscriptable-object
environment_class: type[DockerBuildEnvironment] | type[LocalBuildEnvironment] = None
build_director: BuildDirector = None
config: BuildConfigV1 | BuildConfigV2 = None
Expand Down Expand Up @@ -886,7 +885,7 @@ def store_build_artifacts(self):
build_media_storage.rclone_sync_directory(from_path, to_path)
else:
build_media_storage.sync_directory(from_path, to_path)
except Exception:
except Exception as exc:
# NOTE: the exceptions reported so far are:
# - botocore.exceptions:HTTPClientError
# - botocore.exceptions:ClientError
Expand All @@ -900,7 +899,7 @@ def store_build_artifacts(self):
# Re-raise the exception to fail the build and handle it
# automatically at `on_failure`.
# It will clearly communicate the error to the user.
raise BuildAppError("Error uploading files to the storage.")
raise BuildAppError("Error uploading files to the storage.") from exc

# Delete formats
for media_type in types_to_delete:
Expand All @@ -912,7 +911,7 @@ def store_build_artifacts(self):
)
try:
build_media_storage.delete_directory(media_path)
except Exception:
except Exception as exc:
# NOTE: I didn't find any log line for this case yet
log.exception(
"Error deleting files from storage",
Expand All @@ -922,7 +921,7 @@ def store_build_artifacts(self):
# Re-raise the exception to fail the build and handle it
# automatically at `on_failure`.
# It will clearly communicate the error to the user.
raise BuildAppError("Error deleting files from storage.")
raise BuildAppError("Error deleting files from storage.") from exc

log.info(
"Store build artifacts finished.",
Expand Down

0 comments on commit bfeb6d8

Please sign in to comment.