From 1508796257c305a0b55e1af92056f82d2806f994 Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Thu, 4 Apr 2024 17:28:10 -0400 Subject: [PATCH] fix(remote-build): minor edge cases found with snapcraft (#299) --- craft_application/remote/git.py | 5 +++- craft_application/services/remotebuild.py | 34 +++++++++++++++-------- tests/unit/services/test_remotebuild.py | 17 ++++++++++++ 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/craft_application/remote/git.py b/craft_application/remote/git.py index 8b121b95..735153fd 100644 --- a/craft_application/remote/git.py +++ b/craft_application/remote/git.py @@ -253,7 +253,10 @@ def push_url( # pylint: disable=too-many-branches # temporarily call git directly due to libgit2 bug that unable to push # large repos using https. See https://github.com/libgit2/libgit2/issues/6385 # and https://github.com/snapcore/snapcraft/issues/4478 - cmd: list[str] = ["git", "push", remote_url, refspec, "--progress"] + # Force push in case this repository already exists. The repository is always + # going to exist solely for remote builds, so the only potential issue here is a + # race condition with multiple remote builds on the same machine. + cmd: list[str] = ["git", "push", "--force", remote_url, refspec, "--progress"] if push_tags: cmd.append("--tags") diff --git a/craft_application/services/remotebuild.py b/craft_application/services/remotebuild.py index 0fd4b35f..d22588eb 100644 --- a/craft_application/services/remotebuild.py +++ b/craft_application/services/remotebuild.py @@ -258,7 +258,9 @@ def _ensure_repository( work_tree = WorkTree(self._app.name, self._name, project_dir) work_tree.init_repo() try: - lp_repository = self.lp.new_repository(self._name, self._lp_project.name) + lp_repository = self.lp.new_repository( + self._name, project=self._lp_project.name + ) except launchpadlib.errors.HTTPError: lp_repository = self.lp.get_repository( name=self._name, project=self._lp_project.name @@ -289,21 +291,29 @@ def _ensure_recipe( repository: launchpad.models.GitRepository, **kwargs: Any, # noqa: ANN401 ) -> launchpad.models.Recipe: - """Create a new recipe.""" - # There are 3 different ways this can fail in expected ways, all of them HTTPErrors: - # - # 1. Recipe doesn't exist (or you don't have access to see it) - # 2. You don't have access to delete the recipe - # 3. There is an ongoing build with the recipep - # - # In case 1, we don't care and are about to make the new recipe anyway. - # In the other cases, the errors from `new()` end up being more useful. - with contextlib.suppress(launchpadlib.errors.HTTPError): + """Get a recipe or create it if it doesn't exist.""" + with contextlib.suppress(ValueError): # Recipe doesn't exist recipe = self._get_recipe() recipe.delete() + return self._new_recipe(name, repository, **kwargs) + + def _new_recipe( + self, + name: str, + repository: launchpad.models.GitRepository, + **kwargs: Any, # noqa: ANN401 + ) -> launchpad.models.Recipe: + """Create a new recipe for the given repository.""" + repository.lp_refresh() # Prevents a race condition on new repositories. + git_ref = parse.urlparse(str(repository.git_https_url)).path + "/+ref/main" return self.RecipeClass.new( - self.lp, name, self.lp.username, git_ref=repository.git_https_url, **kwargs + self.lp, + name, + self.lp.username, + git_ref=git_ref, + project=self._lp_project.name, + **kwargs, ) def _get_recipe(self) -> launchpad.models.Recipe: diff --git a/tests/unit/services/test_remotebuild.py b/tests/unit/services/test_remotebuild.py index 3101f172..28aeb8aa 100644 --- a/tests/unit/services/test_remotebuild.py +++ b/tests/unit/services/test_remotebuild.py @@ -186,6 +186,23 @@ def test_ensure_repository_already_exists( assert expiry > datetime.datetime.now(tz=tz) +def test_create_new_recipe(remote_build_service, mock_lp_project): + """Test that _new_recipe works correctly.""" + remote_build_service._lp_project = mock_lp_project + remote_build_service.RecipeClass = mock.Mock() + repo = mock.Mock(git_https_url="https://localhost/~me/some-project/+git/my-repo") + + remote_build_service._new_recipe("test-recipe", repo) + + remote_build_service.RecipeClass.new.assert_called_once_with( + remote_build_service.lp, + "test-recipe", + "craft_test_user", + git_ref="/~me/some-project/+git/my-repo/+ref/main", + project="craft_test_user-craft-remote-build", + ) + + def test_not_setup(remote_build_service): with pytest.raises(RuntimeError): all(remote_build_service.monitor_builds())