-
-
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?
Conversation
Documentation build overview
Show files changed (6 files in total): 📝 6 modified | ➕ 0 added | ➖ 0 deleted
|
Documentation build overview
Show files changed (4 files in total): 📝 4 modified | ➕ 0 added | ➖ 0 deleted
|
latest_version_identifier = ( | ||
self.versions.filter(slug=LATEST, machine=True) | ||
.values_list("identifier", flat=True) | ||
.first() | ||
) |
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.
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).
What happens if the default branch is |
The user who needed this isn't using it, so we can probably remove the feature flag for now, and simplify this logic. |
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.
This looks reasonable, but is a pretty big change to the build process. Hopefully it won't impact too much, but I do worry a bit about how much we're changing here for some edge cases, but I do like the idea of not specifying a branch checkout on initial clone to work around broken latest
, which is a bad UX for users.
|
||
# Search for a branch or tag with the name of the default branch, | ||
# so we can sync latest with it. | ||
original_latest = ( |
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.
Seems we updated get_original_latest_version
, but are also not using it here. Should we be using it here?
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.
No, they are slightly different. One gets the version matching the value of the current LATEST, while the other tries to get the version matching the value of default_branch
(to update LATEST).
Yep, if default_branch isn't set we never reference latest for its branch name when cloning, but we update latest to the new default branch after building, so all links work properly. |
The basic idea is to have the value of latest in sync with the value from the default branch, which we already do. But, in case the default branch is left empty, we were falling back to master, which is just a guess of the default, so instead of guessing the value, we just don't update it if we don't know its value. When cloning the repo, we have access to the default branch, so that's when we update latest for those projects.
Note this is only relevant for projects that aren't linked to a remote repository, since for those we know the default branch. So, when we try to build the latest version of a project that doesn't have a default branch, we:
There's still the problem about latest's identifier being out of sync when the default branch changes, but that's fixed after a new build, and this isn't a new problem (now users aren't blocked at least), and changing the default branch of a repo isn't something users do that often, and this only affects projects that aren't linked to a repository, so I think we are okay with that for now.
This replaces #10927
Closes #12231