From 74c0456534d8793889305dddf41a6117a7fc4e2d Mon Sep 17 00:00:00 2001 From: Ralf Grubenmann Date: Wed, 21 Dec 2022 15:38:45 +0100 Subject: [PATCH] fix(service): use temporary directory to clone project templates on project creation (#3243) Co-authored-by: Ralf Grubenmann --- renku/core/template/template.py | 4 +- renku/domain_model/template.py | 2 +- .../controllers/templates_create_project.py | 48 +++++++++---------- tests/fixtures/templates.py | 2 +- .../test_templates_create_project.py | 2 - tests/service/views/test_templates_views.py | 8 ++-- tests/utils.py | 6 +-- 7 files changed, 34 insertions(+), 38 deletions(-) diff --git a/renku/core/template/template.py b/renku/core/template/template.py index a59d25cc75..927553061c 100644 --- a/renku/core/template/template.py +++ b/renku/core/template/template.py @@ -403,7 +403,7 @@ def get_latest_reference_and_version( else: return (self.reference, self.version) if current_version < Version(self.version) else (reference, version) - def get_template(self, id, reference: Optional[str]) -> Optional["Template"]: + def get_template(self, id, reference: Optional[str]) -> "Template": """Return all available versions for a template id.""" try: return next(t for t in self.templates if t.id == id) @@ -491,7 +491,7 @@ def _has_template_at(self, id: str, reference: str) -> bool: else: return any(t.id == id for t in manifest.templates) - def get_template(self, id, reference: Optional[str]) -> Optional["Template"]: + def get_template(self, id, reference: Optional[str]) -> "Template": """Return a template at a specific reference.""" if reference is not None and reference != self.reference: try: diff --git a/renku/domain_model/template.py b/renku/domain_model/template.py index 59e8d8f2ae..9270e0f372 100644 --- a/renku/domain_model/template.py +++ b/renku/domain_model/template.py @@ -88,7 +88,7 @@ def get_latest_reference_and_version( raise NotImplementedError @abstractmethod - def get_template(self, id, reference: Optional[str]) -> Optional["Template"]: + def get_template(self, id, reference: Optional[str]) -> "Template": """Return a template at a specific reference.""" raise NotImplementedError diff --git a/renku/ui/service/controllers/templates_create_project.py b/renku/ui/service/controllers/templates_create_project.py index a86d7d0c7c..0b80f2e0f0 100644 --- a/renku/ui/service/controllers/templates_create_project.py +++ b/renku/ui/service/controllers/templates_create_project.py @@ -17,19 +17,19 @@ # limitations under the License. """Renku service template create project controller.""" import shutil -from pathlib import Path -from typing import Dict, Optional +from typing import Any, Dict, Optional, cast from marshmallow import EXCLUDE from renku.command.init import create_from_template_local_command +from renku.core import errors +from renku.core.template.template import fetch_templates_source from renku.core.util.contexts import renku_project_context -from renku.domain_model.template import TEMPLATE_MANIFEST, TemplatesManifest +from renku.domain_model.template import Template from renku.infrastructure.repository import Repository from renku.ui.service.config import MESSAGE_PREFIX from renku.ui.service.controllers.api.abstract import ServiceCtrl from renku.ui.service.controllers.api.mixins import RenkuOperationMixin -from renku.ui.service.controllers.utils.project_clone import user_project_clone from renku.ui.service.errors import UserProjectCreationError from renku.ui.service.serializers.templates import ProjectTemplateRequest, ProjectTemplateResponseRPC from renku.ui.service.utils import new_repo_push @@ -45,11 +45,14 @@ class TemplatesCreateProjectCtrl(ServiceCtrl, RenkuOperationMixin): def __init__(self, cache, user_data, request_data): """Construct a templates read manifest controller.""" - self.ctx = TemplatesCreateProjectCtrl.REQUEST_SERIALIZER.load({**user_data, **request_data}, unknown=EXCLUDE) + self.ctx = cast( + Dict[str, Any], + TemplatesCreateProjectCtrl.REQUEST_SERIALIZER.load({**user_data, **request_data}, unknown=EXCLUDE), + ) self.ctx["commit_message"] = f"{MESSAGE_PREFIX} init {self.ctx['project_name']}" super(TemplatesCreateProjectCtrl, self).__init__(cache, user_data, request_data) - self.template: Optional[Dict] = None + self.template: Optional[Template] = None @property def context(self): @@ -59,16 +62,12 @@ def context(self): @property def default_metadata(self): """Default metadata for project creation.""" - automated_update = True - if self.template and "allow_template_update" in self.template: - automated_update = self.template["allow_template_update"] metadata = { "__template_source__": self.ctx["git_url"], "__template_ref__": self.ctx["ref"], "__template_id__": self.ctx["identifier"], "__namespace__": self.ctx["project_namespace"], - "__automated_update__": automated_update, "__repository__": self.ctx["project_repository"], "__sanitized_project_name__": self.ctx["project_name_stripped"], "__project_slug__": self.ctx["project_slug"], @@ -114,26 +113,26 @@ def setup_new_project(self): def setup_template(self): """Reads template manifest.""" - project = user_project_clone(self.user_data, self.ctx) - templates = TemplatesManifest.from_path(Path(project.abs_path) / TEMPLATE_MANIFEST).get_raw_content() + templates_source = fetch_templates_source(source=self.ctx["git_url"], reference=self.ctx["ref"]) identifier = self.ctx["identifier"] - self.template = next((template for template in templates if template["folder"] == identifier), None) - if self.template is None: + try: + self.template = templates_source.get_template(id=identifier, reference=None) + except (errors.InvalidTemplateError, errors.TemplateNotFoundError) as e: raise UserProjectCreationError( error_message=f"the template '{identifier}' does not exist in the target template's repository" - ) + ) from e - repository = Repository(project.abs_path) + repository = Repository(templates_source.path) self.template_version = repository.head.commit.hexsha # Verify missing parameters - template_parameters = self.template.get("variables", {}) + template_parameters = set(p.name for p in self.template.parameters) provided_parameters = {p["key"]: p["value"] for p in self.ctx["parameters"]} - missing_keys = list(template_parameters.keys() - provided_parameters.keys()) + missing_keys = list(template_parameters - provided_parameters.keys()) if len(missing_keys) > 0: raise UserProjectCreationError(error_message=f"the template requires a value for '${missing_keys[0]}'") - return project, provided_parameters + return provided_parameters def new_project_push(self, project_path): """Push new project to the remote.""" @@ -141,23 +140,22 @@ def new_project_push(self, project_path): def new_project(self): """Create new project from template.""" - template_project, provided_parameters = self.setup_template() + provided_parameters = self.setup_template() + assert self.template is not None new_project = self.setup_new_project() new_project_path = new_project.abs_path - source_path = template_project.abs_path / self.ctx["identifier"] - with renku_project_context(new_project_path): create_from_template_local_command().build().execute( - source_path, + self.template.path, name=self.ctx["project_name"], namespace=self.ctx["project_namespace"], metadata=provided_parameters, default_metadata=self.default_metadata, custom_metadata=self.ctx["project_custom_metadata"], template_version=self.template_version, - immutable_template_files=self.template.get("immutable_template_files", []), # type: ignore[union-attr] - automated_template_update=self.template.get("allow_template_update", True), # type: ignore[union-attr] + immutable_template_files=self.template.immutable_files, + automated_template_update=self.template.allow_update, user=self.git_user, initial_branch=self.ctx["initial_branch"], commit_message=self.ctx["commit_message"], diff --git a/tests/fixtures/templates.py b/tests/fixtures/templates.py index 59e19ba3ad..f8c2d01342 100644 --- a/tests/fixtures/templates.py +++ b/tests/fixtures/templates.py @@ -191,7 +191,7 @@ def get_latest_reference_and_version( version = str(max(self._versions)) return version, version - def get_template(self, id, reference: Optional[str]) -> Optional[Template]: + def get_template(self, id, reference: Optional[str]) -> Template: """Return a template at a specific reference.""" if not reference: reference = self.reference diff --git a/tests/service/controllers/test_templates_create_project.py b/tests/service/controllers/test_templates_create_project.py index d53288cb4b..b03bc6c916 100644 --- a/tests/service/controllers/test_templates_create_project.py +++ b/tests/service/controllers/test_templates_create_project.py @@ -66,7 +66,6 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation, "ref", "new_project_url_with_auth", "url_with_auth", - "user_id", } assert expected_context.issubset(set(ctrl.context.keys())) @@ -76,7 +75,6 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation, "__template_ref__", "__template_id__", "__namespace__", - "__automated_update__", "__repository__", "__sanitized_project_name__", "__project_slug__", diff --git a/tests/service/views/test_templates_views.py b/tests/service/views/test_templates_views.py index 569c91487d..e32279631f 100644 --- a/tests/service/views/test_templates_views.py +++ b/tests/service/views/test_templates_views.py @@ -194,7 +194,7 @@ def test_create_project_from_template_failures(svc_client_templates_creation): response = svc_client.post("/templates.create_project", data=json.dumps(payload_missing_project), headers=headers) assert 200 == response.status_code assert {"error"} == set(response.json.keys()) - assert UserProjectCreationError.code == response.json["error"]["code"] + assert UserProjectCreationError.code == response.json["error"]["code"], response.json assert "project name" in response.json["error"]["devMessage"].lower() # NOTE: fail on wrong git url - unexpected when invoked from the UI @@ -204,7 +204,7 @@ def test_create_project_from_template_failures(svc_client_templates_creation): response = svc_client.post("/templates.create_project", data=json.dumps(payload_wrong_repo), headers=headers) assert 200 == response.status_code assert {"error"} == set(response.json.keys()) - assert UserProjectCreationError.code == response.json["error"]["code"] + assert UserProjectCreationError.code == response.json["error"]["code"], response.json assert "git_url" in response.json["error"]["devMessage"] # NOTE: missing fields -- unlikely to happen. If that is the case, we should determine if it's a user error or not @@ -225,7 +225,7 @@ def test_create_project_from_template_failures(svc_client_templates_creation): response = svc_client.post("/templates.create_project", data=json.dumps(payload_fake_id), headers=headers) assert 200 == response.status_code assert {"error"} == set(response.json.keys()) - assert UserProjectCreationError.code == response.json["error"]["code"] + assert UserProjectCreationError.code == response.json["error"]["code"], response.json assert "does not exist" in response.json["error"]["devMessage"] assert fake_identifier in response.json["error"]["devMessage"] @@ -239,6 +239,6 @@ def test_create_project_from_template_failures(svc_client_templates_creation): assert 200 == response.status_code assert {"error"} == set(response.json.keys()) - assert UserProjectCreationError.code == response.json["error"]["code"] + assert UserProjectCreationError.code == response.json["error"]["code"], response.json assert "does not exist" in response.json["error"]["devMessage"] assert fake_identifier in response.json["error"]["devMessage"] diff --git a/tests/utils.py b/tests/utils.py index 505ec2d058..a1ebd5e1b4 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -116,10 +116,10 @@ def modified_environ(*remove, **update): """ env = os.environ update = update or {} - remove_list = list(remove) or [] + remove_set = set(remove or []) # List of environment variables being updated or removed. - stomped = (set(update.keys()) | set(remove_list)) & set(env.keys()) + stomped = (set(update.keys()) | remove_set) & set(env.keys()) # Environment variables and values to restore on exit. update_after = {k: env[k] for k in stomped} # Environment variables and values to remove on exit. @@ -127,7 +127,7 @@ def modified_environ(*remove, **update): try: env.update(update) - [env.pop(k, None) for k in remove_list] + [env.pop(k, None) for k in remove_set] yield finally: env.update(update_after)