-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Build: better support for empty default branch #12462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
dbae5b4
0074e5e
24854de
9572098
082b1be
77ae232
f636eb9
8701e3c
9903db6
335fff8
c7e0087
c9e7138
1723c27
68b4405
722617c
a3374b5
23da415
882d98c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -223,9 +223,26 @@ def checkout(self): | |
| dismissable=True, | ||
| ) | ||
|
|
||
| identifier = self.data.build_commit or self.data.version.identifier | ||
| log.info("Checking out.", identifier=identifier) | ||
| self.vcs_repository.checkout(identifier) | ||
| # Get the default branch of the repository if the project doesn't | ||
| # have an explicit default branch set and we are building latest. | ||
| # The identifier from latest will be updated with this value | ||
| # if the build succeeds. | ||
| is_latest_without_default_branch = ( | ||
| self.data.version.is_machine_latest and not self.data.project.default_branch | ||
| ) | ||
| if is_latest_without_default_branch: | ||
| self.data.default_branch = self.data.build_director.vcs_repository.get_default_branch() | ||
| log.info( | ||
| "Default branch for the repository detected.", | ||
| default_branch=self.data.default_branch, | ||
| ) | ||
|
|
||
| # We can skip the checkout step since we just cloned the repository, | ||
| # and the default branch is already checked out. | ||
| if not is_latest_without_default_branch: | ||
| identifier = self.data.build_commit or self.data.version.identifier | ||
| log.info("Checking out.", identifier=identifier) | ||
| self.vcs_repository.checkout(identifier) | ||
|
Comment on lines
+240
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would remove this conditional here and always execute this command, for simplification of the workflow. It's better to be explicit here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems this won't be executed only at most, once, per project. Starting from the second time, the project will have I would simplify this and execute this command always to reduce complexity here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have the name of the branch for those projects, omitting this step is the purpose of this PR.
No, default_branch is kept empty for those projects. |
||
|
|
||
| # The director is responsible for understanding which config file to use for a build. | ||
| # In order to reproduce a build 1:1, we may use readthedocs_yaml_path defined by the build | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1175,15 +1175,21 @@ def get_original_latest_version(self): | |
| When latest is machine created, it's basically an alias | ||
| for the default branch/tag (like main/master), | ||
|
|
||
| Returns None if the current default version doesn't point to a valid version. | ||
| Returns None if latest doesn't point to a valid version, | ||
| or if isn't managed by RTD (machine=False). | ||
| """ | ||
| default_version_name = self.get_default_branch() | ||
| # For latest, the identifier is the name of the branch/tag. | ||
| latest_version_identifier = ( | ||
| self.versions.filter(slug=LATEST, machine=True) | ||
| .values_list("identifier", flat=True) | ||
| .first() | ||
| ) | ||
|
Comment on lines
+1182
to
+1186
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is introducing an extra query for each time latest is returned in a response, since we now rely on latest having the correct value of the default branch (similar to get_original_stable_version). |
||
| if not latest_version_identifier: | ||
| return None | ||
| return ( | ||
| self.versions(manager=INTERNAL) | ||
| .exclude(slug=LATEST) | ||
| .filter( | ||
| verbose_name=default_version_name, | ||
| ) | ||
| .filter(verbose_name=latest_version_identifier) | ||
| .first() | ||
| ) | ||
|
|
||
|
|
@@ -1201,8 +1207,22 @@ def update_latest_version(self): | |
| return | ||
|
|
||
| # default_branch can be a tag or a branch name! | ||
| default_version_name = self.get_default_branch() | ||
| original_latest = self.get_original_latest_version() | ||
| default_version_name = self.get_default_branch(fallback_to_vcs=False) | ||
| # If the default_branch is not set, it means that the user | ||
| # wants to use the default branch of the respository, but | ||
| # we don't know what that is here, `latest` will be updated | ||
| # on the next build. | ||
| if not default_version_name: | ||
| return | ||
|
|
||
| # Search for a branch or tag with the name of the default branch, | ||
| # so we can sync latest with it. | ||
| original_latest = ( | ||
ericholscher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| self.versions(manager=INTERNAL) | ||
| .exclude(slug=LATEST) | ||
| .filter(verbose_name=default_version_name) | ||
| .first() | ||
| ) | ||
| latest.verbose_name = LATEST_VERBOSE_NAME | ||
| latest.type = original_latest.type if original_latest else BRANCH | ||
| # For latest, the identifier is the name of the branch/tag. | ||
|
|
@@ -1302,14 +1322,22 @@ def get_default_version(self): | |
| return self.default_version | ||
| return LATEST | ||
|
|
||
| def get_default_branch(self): | ||
| """Get the version representing 'latest'.""" | ||
| def get_default_branch(self, fallback_to_vcs=True): | ||
| """ | ||
| Get the name of the branch or tag that the user wants to use as 'latest'. | ||
|
|
||
| In case the user explicitly set a default branch, we use that, | ||
| otherwise we try to get it from the remote repository. | ||
| """ | ||
| if self.default_branch: | ||
| return self.default_branch | ||
|
|
||
| if self.remote_repository and self.remote_repository.default_branch: | ||
| return self.remote_repository.default_branch | ||
|
|
||
| if not fallback_to_vcs: | ||
| return None | ||
|
|
||
| vcs_class = self.vcs_class() | ||
| if vcs_class: | ||
| return vcs_class.fallback_branch | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ | |
| from readthedocs.builds import tasks as build_tasks | ||
| from readthedocs.builds.constants import ARTIFACT_TYPES | ||
| from readthedocs.builds.constants import ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT | ||
| from readthedocs.builds.constants import BRANCH | ||
| from readthedocs.builds.constants import BUILD_FINAL_STATES | ||
| from readthedocs.builds.constants import BUILD_STATE_BUILDING | ||
| from readthedocs.builds.constants import BUILD_STATE_CANCELLED | ||
|
|
@@ -117,6 +118,10 @@ class TaskData: | |
| config: BuildConfigV2 = None | ||
| project: APIProject = None | ||
| version: APIVersion = None | ||
| # Default branch for the repository. | ||
| # Only set when building the latest version, and the project | ||
| # doesn't have an explicit default branch. | ||
| default_branch: str | None = None | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we call it more explicitly like |
||
|
|
||
| # Dictionary returned from the API. | ||
| build: dict = field(default_factory=dict) | ||
|
|
@@ -665,18 +670,22 @@ def on_success(self, retval, task_id, args, kwargs): | |
| # NOTE: we are updating the db version instance *only* when | ||
| # TODO: remove this condition and *always* update the DB Version instance | ||
| if "html" in valid_artifacts: | ||
| data = { | ||
| "built": True, | ||
| "documentation_type": self.data.version.documentation_type, | ||
| "has_pdf": "pdf" in valid_artifacts, | ||
| "has_epub": "epub" in valid_artifacts, | ||
| "has_htmlzip": "htmlzip" in valid_artifacts, | ||
| "build_data": self.data.version.build_data, | ||
| "addons": self.data.version.addons, | ||
| } | ||
| # Update the latest version to point to the current VCS default branch | ||
| # if the project doesn't have an explicit default branch set. | ||
| if self.data.default_branch: | ||
| data["identifier"] = self.data.default_branch | ||
| data["type"] = BRANCH | ||
| try: | ||
| self.data.api_client.version(self.data.version.pk).patch( | ||
| { | ||
| "built": True, | ||
| "documentation_type": self.data.version.documentation_type, | ||
| "has_pdf": "pdf" in valid_artifacts, | ||
| "has_epub": "epub" in valid_artifacts, | ||
| "has_htmlzip": "htmlzip" in valid_artifacts, | ||
| "build_data": self.data.version.build_data, | ||
| "addons": self.data.version.addons, | ||
| } | ||
| ) | ||
| self.data.api_client.version(self.data.version.pk).patch(data) | ||
| except HttpClientError: | ||
| # NOTE: I think we should fail the build if we cannot update | ||
| # the version at this point. Otherwise, we will have inconsistent data | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to save this in the database. Why we are not always cloning the repository without the default branch set if it will clone the correct one?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are confusing the default_branch from the project model. This is the default branch of the repository, which is used to keep the identifier of latest in sync, not the attribute from the project model.