Webhooks: refactor branch/tag building #12014
Conversation
Extracted from #11942
| return ref.removeprefix(heads_prefix), BRANCH | ||
| if ref.startswith(tags_prefix): | ||
| return ref.removeprefix(tags_prefix), TAG |
There was a problem hiding this comment.
Adding parenthesis as it's easier to read.
| return ref.removeprefix(heads_prefix), BRANCH | |
| if ref.startswith(tags_prefix): | |
| return ref.removeprefix(tags_prefix), TAG | |
| return (ref.removeprefix(heads_prefix), BRANCH) | |
| if ref.startswith(tags_prefix): | |
| return (ref.removeprefix(tags_prefix), TAG) |
There was a problem hiding this comment.
Think black will just remove these?
readthedocs/core/views/hooks.py
Outdated
| @@ -15,7 +15,7 @@ | |||
| log = structlog.get_logger(__name__) | |||
|
|
|||
|
|
|||
| def _build_version(project, slug, already_built=()): | |||
| def _build_version(project, version): | |||
| """ | |||
| Where we actually trigger builds for a project and slug. | |||
There was a problem hiding this comment.
| Where we actually trigger builds for a project and slug. | |
| Where we actually trigger builds for a project and version. |
readthedocs/core/views/hooks.py
Outdated
| @@ -27,44 +27,43 @@ def _build_version(project, slug, already_built=()): | |||
| # Previously we were building the latest version (inactive or active) | |||
| # when building the default version, | |||
| # some users may have relied on this to update the version list #4450 | |||
| version = project.versions.filter(active=True, slug=slug).first() | |||
| if version and slug not in already_built: | |||
| if version.active: | |||
| log.info( | |||
| "Building.", | |||
There was a problem hiding this comment.
| "Building.", | |
| "Triggering build.", |
| @@ -15,7 +15,7 @@ | |||
| log = structlog.get_logger(__name__) | |||
|
|
|||
|
|
|||
| def _build_version(project, slug, already_built=()): | |||
| def _build_version(project, version): | |||
There was a problem hiding this comment.
If we are passing the version object, project is not needed anymore.
There was a problem hiding this comment.
We usually have the project already loaded outside the version, relying on the version only will result in another queryset, we pass the project and version in several parts of the codebase.
| to_build.add(ret) | ||
| if version.slug in to_build: | ||
| continue | ||
| if _build_version(project, version): |
There was a problem hiding this comment.
I don't like too much when we use a function in a conditional that trigger things behind the scenes. I would split the boolean check and trigger the build inside the conditional instead. We mentioned something similar in other opportunity cc @ericholscher
I'd rewrite this as:
if should_build_version(project, version):
trigger_build(project=project, version=version)
to_build.add(version.slug)This is a lot clearer, easier to read and simple to follow.
There was a problem hiding this comment.
That would just mean to extract the whole _build_version function into the caller or have a one-liner helper function... Not really follow what's the problem here? The return value hints if the version was built or not, we already use this pattern in the codebase to hint if an operation was successful or not.
| # so we don't have a way to match it by name. | ||
| # We should do another lookup to get the original stable version, | ||
| # or change our logic to store the tags name in the identifier of stable. | ||
| | Q(identifier=name, machine=True) |
There was a problem hiding this comment.
Why we were using remotes/origin/%s and origin/%s previously and we are not now? I'm confused with this method and I'm not sure the logic will be the same. Do we have enough tests for this?
There was a problem hiding this comment.
We used to save the branches with remote/origin as the identifier, and verbose_name without it, identifier and verbose name are always the same for branches now (this was changed a long time ago when we switched to gitpython...). Anyway, this is always using the verbose name, so even if there are ancient versions that include remote/origin in the identifier, this won't be a breaking change.
Extracted from #11942