-
Notifications
You must be signed in to change notification settings - Fork 2
kpd: add optional mirror support #16
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
Changes from all commits
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 |
---|---|---|
|
@@ -41,5 +41,7 @@ | |
"github_oauth_token": "<TOKEN>" | ||
} | ||
}, | ||
"base_directory": "/tmp/repos" | ||
"base_directory": "/tmp/repos", | ||
"mirror_dir": "/mirror/", | ||
"linux_clone": true | ||
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.
How about we change this option to smth like 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. Sure.. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -547,6 +547,8 @@ def __init__( | |
app_auth: Optional[Auth.AppInstallationAuth] = None, | ||
email: Optional[EmailConfig] = None, | ||
http_retries: Optional[int] = None, | ||
linux_clone: bool = False, | ||
mirror_dir: Optional[str] = None, | ||
) -> None: | ||
super().__init__( | ||
repo_url=repo_url, | ||
|
@@ -559,6 +561,8 @@ def __init__( | |
self.email = email | ||
|
||
self.log_extractor = log_extractor | ||
self.mirror_dir = mirror_dir | ||
self.linux_clone = linux_clone | ||
self.ci_repo_url = ci_repo_url | ||
self.ci_repo_dir = _uniq_tmp_folder(ci_repo_url, ci_branch, base_directory) | ||
self.ci_branch = ci_branch | ||
|
@@ -682,9 +686,28 @@ def do_sync(self) -> None: | |
def full_sync(self, path: str, url: str, branch: str) -> git.Repo: | ||
logging.info(f"Doing full clone from {redact_url(url)}, branch: {branch}") | ||
|
||
multi_opts: Optional[List[str]] = None | ||
if self.mirror_dir: | ||
upstream_name = os.path.basename(self.upstream_url) | ||
reference_path = os.path.join(self.mirror_dir, upstream_name) | ||
fallback = None | ||
if self.linux_clone: | ||
fallback = os.path.join(self.mirror_dir, "linux.git") | ||
if ( | ||
not os.path.exists(reference_path) | ||
and fallback | ||
and os.path.exists(fallback) | ||
): | ||
reference_path = fallback | ||
if os.path.exists(reference_path): | ||
multi_opts = ["--reference", reference_path] | ||
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. TIL nit: Is it more appropriate to use 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. Was not aware of --reference-if-able. yes. Good. |
||
|
||
with HistogramMetricTimer(git_clone_duration, {"branch": branch}): | ||
shutil.rmtree(path, ignore_errors=True) | ||
repo = git.Repo.clone_from(url, path) | ||
if multi_opts: | ||
repo = git.Repo.clone_from(url, path, multi_options=multi_opts) | ||
else: | ||
repo = git.Repo.clone_from(url, path) | ||
Comment on lines
+707
to
+710
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 think it makes a lot of sense to always have If a mirror doesn't exist, clone it from scratch to What do you think? 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. Makes perfect sense to me. I'll see if codex can parse, "look a this URL and make all the suggested changes". |
||
_reset_repo(repo, f"origin/{branch}") | ||
|
||
git_clone_counter.add(1, {"branch": branch}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,8 @@ def __init__( | |
ci_branch=branch_config.ci_branch, | ||
log_extractor=_log_extractor_from_project(kpd_config.patchwork.project), | ||
base_directory=kpd_config.base_directory, | ||
mirror_dir=kpd_config.mirror_dir, | ||
linux_clone=kpd_config.linux_clone, | ||
Comment on lines
+117
to
+118
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. Since new config options are parameters of 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. Sure. |
||
http_retries=http_retries, | ||
github_oauth_token=branch_config.github_oauth_token, | ||
app_auth=github_app_auth_from_branch_config(branch_config), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,7 @@ | |
TEST_CI_REPO_URL = f"https://user:[email protected]/ci-org/{TEST_CI_REPO}" | ||
TEST_CI_BRANCH = "test_ci_branch" | ||
TEST_BASE_DIRECTORY = "/repos" | ||
TEST_MIRROR_DIRECTORY = "/mirror" | ||
TEST_BRANCH = "test-branch" | ||
TEST_CONFIG: Dict[str, Any] = { | ||
"version": 2, | ||
|
@@ -124,6 +125,8 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: | |
"ci_branch": TEST_CI_BRANCH, | ||
"log_extractor": DefaultGithubLogExtractor(), | ||
"base_directory": TEST_BASE_DIRECTORY, | ||
"mirror_dir": None, | ||
"linux_clone": False, | ||
} | ||
presets.update(kwargs) | ||
|
||
|
@@ -464,6 +467,56 @@ def test_fetch_repo_path_exists_git_exception(self) -> None: | |
self._bw.fetch_repo(*fetch_params) | ||
fr.assert_called_once_with(*fetch_params) | ||
|
||
def test_full_sync_with_mirror_dir(self) -> None: | ||
bw = BranchWorkerMock(mirror_dir=TEST_MIRROR_DIRECTORY) | ||
reference = os.path.join( | ||
TEST_MIRROR_DIRECTORY, os.path.basename(TEST_UPSTREAM_REPO_URL) | ||
) | ||
with ( | ||
patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists, | ||
patch("kernel_patches_daemon.branch_worker.shutil.rmtree") as rm, | ||
): | ||
exists.side_effect = lambda p: p == reference | ||
bw.upstream_url = TEST_UPSTREAM_REPO_URL | ||
bw.full_sync("somepath", "giturl", "branch") | ||
self._git_repo_mock.clone_from.assert_called_once_with( | ||
"giturl", | ||
"somepath", | ||
multi_options=["--reference", reference], | ||
) | ||
|
||
def test_full_sync_with_linux_mirror_fallback(self) -> None: | ||
bw = BranchWorkerMock(mirror_dir=TEST_MIRROR_DIRECTORY, linux_clone=True) | ||
fallback = os.path.join(TEST_MIRROR_DIRECTORY, "linux.git") | ||
with ( | ||
patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists, | ||
patch("kernel_patches_daemon.branch_worker.shutil.rmtree") as rm, | ||
): | ||
exists.side_effect = lambda p: p == fallback | ||
bw.upstream_url = TEST_UPSTREAM_REPO_URL | ||
bw.full_sync("somepath", "giturl", "branch") | ||
self._git_repo_mock.clone_from.assert_called_once_with( | ||
"giturl", | ||
"somepath", | ||
multi_options=["--reference", fallback], | ||
) | ||
|
||
def test_full_sync_without_linux_mirror_fallback(self) -> None: | ||
bw = BranchWorkerMock(mirror_dir=TEST_MIRROR_DIRECTORY, linux_clone=False) | ||
fallback = os.path.join(TEST_MIRROR_DIRECTORY, "linux.git") | ||
with ( | ||
patch("kernel_patches_daemon.branch_worker.os.path.exists") as exists, | ||
patch("kernel_patches_daemon.branch_worker.shutil.rmtree") as rm, | ||
): | ||
exists.side_effect = lambda p: p == fallback | ||
bw.upstream_url = TEST_UPSTREAM_REPO_URL | ||
bw.full_sync("somepath", "giturl", "branch") | ||
# Without linux_mirror we should not use fallback | ||
self._git_repo_mock.clone_from.assert_called_once_with( | ||
"giturl", | ||
"somepath", | ||
) | ||
|
||
def test_expire_branches(self) -> None: | ||
"""Only the branch that matches pattern and is expired should be deleted""" | ||
not_expired_time = datetime.fromtimestamp(3 * BRANCH_TTL) | ||
|
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 think this section belongs to the README file.
It would be great to have a separate document about the config, if you don't mind making a draft of that.
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.
Sure