From 0eaf204365d38bbf82bd2d0df357abbf61c18548 Mon Sep 17 00:00:00 2001 From: Ralf Grubenmann Date: Fri, 26 May 2023 16:14:31 +0200 Subject: [PATCH] fix(service): fix working on branches in the core-svc (#3472) --- renku/ui/service/cache/models/job.py | 1 + renku/ui/service/controllers/api/mixins.py | 22 +++++++++---------- .../controllers/cache_migrations_check.py | 2 +- .../controllers/templates_create_project.py | 4 ++-- .../controllers/templates_read_manifest.py | 2 +- .../controllers/utils/project_clone.py | 2 +- .../controllers/utils/remote_project.py | 2 +- .../service/gateways/gitlab_api_provider.py | 8 +++---- .../ui/service/interfaces/git_api_provider.py | 2 +- renku/ui/service/serializers/cache.py | 13 ----------- renku/ui/service/serializers/common.py | 15 +++++++++++-- renku/ui/service/serializers/datasets.py | 17 ++++---------- renku/ui/service/serializers/templates.py | 1 - tests/fixtures/templates.py | 6 ++--- .../test_templates_create_project.py | 4 ++-- .../controllers/utils/test_remote_project.py | 6 ++--- tests/service/jobs/test_datasets.py | 22 +++++++++---------- tests/service/jobs/test_project.py | 4 +++- tests/service/views/test_templates_views.py | 2 +- 19 files changed, 63 insertions(+), 72 deletions(-) diff --git a/renku/ui/service/cache/models/job.py b/renku/ui/service/cache/models/job.py index fc41e639cd..38135980a4 100644 --- a/renku/ui/service/cache/models/job.py +++ b/renku/ui/service/cache/models/job.py @@ -43,6 +43,7 @@ class Job(Model): state = TextField() extras = JSONField() client_extras = TextField() + branch = TextField() ctrl_context = JSONField() ctrl_result = JSONField() diff --git a/renku/ui/service/controllers/api/mixins.py b/renku/ui/service/controllers/api/mixins.py index 0f72c21951..934f98d9ba 100644 --- a/renku/ui/service/controllers/api/mixins.py +++ b/renku/ui/service/controllers/api/mixins.py @@ -169,8 +169,8 @@ def execute_op(self): "git_url": self.request_data["git_url"], } - if "ref" in self.request_data: - clone_context["ref"] = self.request_data["ref"] + if "branch" in self.request_data: + clone_context["branch"] = self.request_data["branch"] # NOTE: If we want to migrate project, then we need to do full clone. # This operation can take very long time, and as such is expected @@ -185,27 +185,27 @@ def execute_op(self): if not project.initialized: raise UninitializedProject(project.abs_path) else: - ref = self.request_data.get("ref", None) + branch = self.request_data.get("branch", None) - if ref: + if branch: with Repository(project.abs_path) as repository: - if ref != repository.active_branch.name: + if branch != repository.active_branch.name: # NOTE: Command called for different branch than the one used in cache, change branch if len(repository.remotes) != 1: raise RenkuException("Couldn't find remote for project in cache.") origin = repository.remotes[0] - remote_branch = f"{origin}/{ref}" + remote_branch = f"{origin}/{branch}" with project.write_lock(): - # NOTE: Add new ref to remote branches - repository.run_git_command("remote", "set-branches", "--add", origin, ref) + # NOTE: Add new branch to remote branches + repository.run_git_command("remote", "set-branches", "--add", origin, branch) if self.migrate_project or self.clone_depth == PROJECT_CLONE_NO_DEPTH: - repository.fetch(origin, ref) + repository.fetch(origin, branch) else: - repository.fetch(origin, ref, depth=self.clone_depth) + repository.fetch(origin, branch, depth=self.clone_depth) # NOTE: Switch to new ref - repository.run_git_command("checkout", "--track", "-f", "-b", ref, remote_branch) + repository.run_git_command("checkout", "--track", "-f", "-b", branch, remote_branch) # NOTE: cleanup remote branches in case a remote was deleted (fetch fails otherwise) repository.run_git_command("remote", "prune", origin) diff --git a/renku/ui/service/controllers/cache_migrations_check.py b/renku/ui/service/controllers/cache_migrations_check.py index 24a5b61c4e..61cfb14da6 100644 --- a/renku/ui/service/controllers/cache_migrations_check.py +++ b/renku/ui/service/controllers/cache_migrations_check.py @@ -66,7 +66,7 @@ def _fast_op_without_cache(self): ], tempdir_path, remote=self.ctx["git_url"], - ref=self.request_data.get("ref", None), + branch=self.request_data.get("branch", None), token=self.user_data.get("token", None), ) with renku_project_context(tempdir_path): diff --git a/renku/ui/service/controllers/templates_create_project.py b/renku/ui/service/controllers/templates_create_project.py index 051f6e460d..2d6842a188 100644 --- a/renku/ui/service/controllers/templates_create_project.py +++ b/renku/ui/service/controllers/templates_create_project.py @@ -65,7 +65,7 @@ def default_metadata(self): metadata = { "__template_source__": self.ctx["git_url"], - "__template_ref__": self.ctx["ref"], + "__template_ref__": self.ctx["branch"], "__template_id__": self.ctx["identifier"], "__namespace__": self.ctx["project_namespace"], "__repository__": self.ctx["project_repository"], @@ -115,7 +115,7 @@ def setup_new_project(self): def setup_template(self): """Reads template manifest.""" - templates_source = fetch_templates_source(source=self.ctx["git_url"], reference=self.ctx["ref"]) + templates_source = fetch_templates_source(source=self.ctx["git_url"], reference=self.ctx["branch"]) identifier = self.ctx["identifier"] try: self.template = templates_source.get_template(id=identifier, reference=None) diff --git a/renku/ui/service/controllers/templates_read_manifest.py b/renku/ui/service/controllers/templates_read_manifest.py index 463b110977..d46d4e2e32 100644 --- a/renku/ui/service/controllers/templates_read_manifest.py +++ b/renku/ui/service/controllers/templates_read_manifest.py @@ -50,7 +50,7 @@ def template_manifest(self): """Reads template manifest.""" from PIL import Image - templates_source = fetch_templates_source(source=self.ctx["git_url"], reference=self.ctx["ref"]) + templates_source = fetch_templates_source(source=self.ctx["git_url"], reference=self.ctx["branch"]) manifest = templates_source.manifest.get_raw_content() # NOTE: convert icons to base64 diff --git a/renku/ui/service/controllers/utils/project_clone.py b/renku/ui/service/controllers/utils/project_clone.py index 3ab5652d88..cf7b320198 100644 --- a/renku/ui/service/controllers/utils/project_clone.py +++ b/renku/ui/service/controllers/utils/project_clone.py @@ -73,7 +73,7 @@ def user_project_clone(cache, user_data, project_data): "user.email": project_data["email"], "pull.rebase": False, }, - checkout_revision=project_data["ref"], + checkout_revision=project_data["branch"], ) ).output project.save() diff --git a/renku/ui/service/controllers/utils/remote_project.py b/renku/ui/service/controllers/utils/remote_project.py index f5775f14a5..5154b672c4 100644 --- a/renku/ui/service/controllers/utils/remote_project.py +++ b/renku/ui/service/controllers/utils/remote_project.py @@ -45,7 +45,7 @@ def __init__(self, user_data, request_data): self.ctx = ProjectCloneContext().load({**user_data, **request_data}, unknown=EXCLUDE) self.git_url = self.ctx["url_with_auth"] - self.branch = self.ctx["ref"] + self.branch = self.ctx["branch"] @property def remote_url(self): diff --git a/renku/ui/service/gateways/gitlab_api_provider.py b/renku/ui/service/gateways/gitlab_api_provider.py index 8d14f1b00c..eeb6cbd0b5 100644 --- a/renku/ui/service/gateways/gitlab_api_provider.py +++ b/renku/ui/service/gateways/gitlab_api_provider.py @@ -47,11 +47,11 @@ def download_files_from_api( target_folder: Union[Path, str], remote: str, token: str, - ref: Optional[str] = None, + branch: Optional[str] = None, ): """Download files through a remote Git API.""" - if not ref: - ref = "HEAD" + if not branch: + branch = "HEAD" target_folder = Path(target_folder) @@ -82,7 +82,7 @@ def download_files_from_api( try: with open(full_path, "wb") as f: - project.files.raw(file_path=str(path), ref=ref, streamed=True, action=f.write) + project.files.raw(file_path=str(path), ref=branch, streamed=True, action=f.write) result_paths.append(full_path) except gitlab.GitlabGetError: diff --git a/renku/ui/service/interfaces/git_api_provider.py b/renku/ui/service/interfaces/git_api_provider.py index e0e4ce9d46..77fa210705 100644 --- a/renku/ui/service/interfaces/git_api_provider.py +++ b/renku/ui/service/interfaces/git_api_provider.py @@ -30,7 +30,7 @@ def download_files_from_api( target_folder: Union[Path, str], remote: str, token: str, - ref: Optional[str] = None, + branch: Optional[str] = None, ): """Download files through a remote Git API.""" raise NotImplementedError() diff --git a/renku/ui/service/serializers/cache.py b/renku/ui/service/serializers/cache.py index 619a8ef658..1a251f3882 100644 --- a/renku/ui/service/serializers/cache.py +++ b/renku/ui/service/serializers/cache.py @@ -131,7 +131,6 @@ class RepositoryCloneRequest(RemoteRepositorySchema): """Request schema for repository clone.""" depth = fields.Integer(metadata={"description": "Git fetch depth"}, load_default=PROJECT_CLONE_DEPTH_DEFAULT) - ref = fields.String(metadata={"description": "Repository reference (branch, commit or tag)"}, load_default=None) class ProjectCloneContext(RepositoryCloneRequest): @@ -240,18 +239,6 @@ class ProjectMigrateRequest(AsyncSchema, LocalRepositorySchema, RemoteRepository skip_docker_update = fields.Boolean(dump_default=False) skip_migrations = fields.Boolean(dump_default=False) - @pre_load() - def handle_ref(self, data, **kwargs): - """Handle ref and branch.""" - - # Backward compatibility: branch and ref were both used. Let's keep branch as the exposed field - # even if interally it gets converted to "ref" later. - if data.get("ref"): - data["branch"] = data["ref"] - del data["ref"] - - return data - class ProjectMigrateResponse(RenkuSyncSchema): """Response schema for project migrate.""" diff --git a/renku/ui/service/serializers/common.py b/renku/ui/service/serializers/common.py index c246b9dad1..7c145bf7f6 100644 --- a/renku/ui/service/serializers/common.py +++ b/renku/ui/service/serializers/common.py @@ -19,7 +19,7 @@ from datetime import datetime import yagup -from marshmallow import Schema, fields, validates +from marshmallow import Schema, fields, pre_load, validates from renku.ui.service.errors import UserRepoUrlInvalidError from renku.ui.service.serializers.rpc import JsonRPCResponse @@ -52,7 +52,18 @@ def validate_git_url(self, value): class RemoteRepositorySchema(RemoteRepositoryBaseSchema): """Schema for tracking a remote repository and branch.""" - branch = fields.String(metadata={"description": "Remote git branch."}) + branch = fields.String(load_default=None, metadata={"description": "Remote git branch (or tag or commit SHA)."}) + + @pre_load + def set_branch_from_ref(self, data, **kwargs): + """Set `branch` field from `ref` if present.""" + if "ref" in data and not data.get("branch"): + # Backward compatibility: branch and ref were both used. Let's keep branch as the exposed field + # even if internally it gets converted to "ref" later. + data["branch"] = data["ref"] + del data["ref"] + + return data class AsyncSchema(Schema): diff --git a/renku/ui/service/serializers/datasets.py b/renku/ui/service/serializers/datasets.py index 97c4504840..56fc98fd30 100644 --- a/renku/ui/service/serializers/datasets.py +++ b/renku/ui/service/serializers/datasets.py @@ -32,12 +32,6 @@ from renku.ui.service.serializers.rpc import JsonRPCResponse -class DatasetRefSchema(Schema): - """Schema for specifying a reference.""" - - ref = fields.String(metadata={"description": "Target reference."}) - - class DatasetNameSchema(Schema): """Schema for dataset name.""" @@ -53,7 +47,7 @@ class DatasetDetailsRequest(DatasetDetails): class DatasetCreateRequest( - AsyncSchema, DatasetDetailsRequest, DatasetRefSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema + AsyncSchema, DatasetDetailsRequest, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema ): """Request schema for a dataset create view.""" @@ -75,7 +69,7 @@ class DatasetCreateResponseRPC(JsonRPCResponse): class DatasetRemoveRequest( - AsyncSchema, DatasetNameSchema, DatasetRefSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema + AsyncSchema, DatasetNameSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema ): """Request schema for a dataset remove.""" @@ -99,9 +93,7 @@ class DatasetAddFile(Schema): job_id = fields.String() -class DatasetAddRequest( - AsyncSchema, DatasetNameSchema, DatasetRefSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema -): +class DatasetAddRequest(AsyncSchema, DatasetNameSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema): """Request schema for a dataset add file view.""" files = fields.List(fields.Nested(DatasetAddFile), required=True) @@ -203,7 +195,6 @@ class DatasetEditRequest( AsyncSchema, DatasetDetailsRequest, DatasetNameSchema, - DatasetRefSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema, @@ -240,7 +231,7 @@ class DatasetEditResponseRPC(JsonRPCResponse): class DatasetUnlinkRequest( - AsyncSchema, DatasetNameSchema, DatasetRefSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema + AsyncSchema, DatasetNameSchema, LocalRepositorySchema, RemoteRepositorySchema, MigrateSchema ): """Dataset unlink file request.""" diff --git a/renku/ui/service/serializers/templates.py b/renku/ui/service/serializers/templates.py index ef3cb5a3b8..adae0d02b6 100644 --- a/renku/ui/service/serializers/templates.py +++ b/renku/ui/service/serializers/templates.py @@ -32,7 +32,6 @@ class ManifestTemplatesRequest(RepositoryCloneRequest): """Request schema for listing manifest templates.""" url = fields.String(required=True) - ref = fields.String(load_default=None) depth = fields.Integer(load_default=TEMPLATE_CLONE_DEPTH_DEFAULT) @pre_load() diff --git a/tests/fixtures/templates.py b/tests/fixtures/templates.py index c74736d500..d65e88a5a8 100644 --- a/tests/fixtures/templates.py +++ b/tests/fixtures/templates.py @@ -59,7 +59,7 @@ def template(template_metadata): "url": "https://github.com/SwissDataScienceCenter/renku-project-template", "id": "python-minimal", "index": 1, - "ref": "master", + "branch": "master", # TODO: Add template parameters here once parameters are added to the template. "metadata": {}, "default_metadata": template_metadata, @@ -84,7 +84,7 @@ def project_init(template): "init_custom": [ "init", "--template-ref", - template["ref"], + template["branch"], "--template-id", "python-minimal", data["test_project"], @@ -92,7 +92,7 @@ def project_init(template): "init_custom_template": ( "https://gitlab.dev.renku.ch/renku-python-integration-tests/core-it-template-variable-test-project" ), - "remote": ["--template-source", template["url"], "--template-ref", template["ref"]], + "remote": ["--template-source", template["url"], "--template-ref", template["branch"]], "id": ["--template-id", template["id"]], "force": ["--force"], "parameters": ["--parameter", "p1=v1", "--parameter", "p2=v2"], diff --git a/tests/service/controllers/test_templates_create_project.py b/tests/service/controllers/test_templates_create_project.py index f70c234d01..d2ab69c80c 100644 --- a/tests/service/controllers/test_templates_create_project.py +++ b/tests/service/controllers/test_templates_create_project.py @@ -62,7 +62,7 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation, "git_url", "project_name_stripped", "depth", - "ref", + "branch", "new_project_url_with_auth", "url_with_auth", } @@ -83,7 +83,7 @@ def test_template_create_project_ctrl(ctrl_init, svc_client_templates_creation, expected_metadata.add("__renku_version__") assert expected_metadata == set(received_metadata.keys()) assert payload["url"] == received_metadata["__template_source__"] - assert payload["ref"] == received_metadata["__template_ref__"] + assert payload["branch"] == received_metadata["__template_ref__"] assert payload["identifier"] == received_metadata["__template_id__"] assert payload["project_namespace"] == received_metadata["__namespace__"] assert payload["project_repository"] == received_metadata["__repository__"] diff --git a/tests/service/controllers/utils/test_remote_project.py b/tests/service/controllers/utils/test_remote_project.py index f8f91b4ba9..1731e89c1f 100644 --- a/tests/service/controllers/utils/test_remote_project.py +++ b/tests/service/controllers/utils/test_remote_project.py @@ -55,13 +55,13 @@ def test_project_metadata_custom_remote(): request_data = { "git_url": "https://gitlab.dev.renku.ch/renku-python-integration-tests/import-me", - "ref": "my-branch", + "branch": "my-branch", } ctrl = RemoteProject(user_data, request_data) - ref = ctrl.ctx["ref"] + branch = ctrl.ctx["branch"] - assert request_data["ref"] == ref + assert request_data["branch"] == branch def test_project_metadata_remote_err(): diff --git a/tests/service/jobs/test_datasets.py b/tests/service/jobs/test_datasets.py index c26f6ce003..041013023f 100644 --- a/tests/service/jobs/test_datasets.py +++ b/tests/service/jobs/test_datasets.py @@ -302,7 +302,7 @@ def test_delay_add_file_job(svc_client_cache, it_remote_repo_url_temp_branch, vi context = DatasetAddRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": uuid.uuid4().hex, # NOTE: We test with this only to check that recursive invocation is being prevented. "is_delayed": True, @@ -346,7 +346,7 @@ def test_delay_add_file_job_failure(svc_client_cache, it_remote_repo_url_temp_br context = DatasetAddRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": uuid.uuid4().hex, # NOTE: We test with this only to check that recursive invocation is being prevented. "is_delayed": True, @@ -414,7 +414,7 @@ def test_delay_create_dataset_job(svc_client_cache, it_remote_repo_url_temp_bran context = DatasetCreateRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": uuid.uuid4().hex, # NOTE: We test with this only to check that recursive invocation is being prevented. "is_delayed": True, @@ -451,7 +451,7 @@ def test_delay_create_dataset_failure(svc_client_cache, it_remote_repo_url_temp_ context = DatasetCreateRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": uuid.uuid4().hex, # NOTE: We test with this only to check that recursive invocation is being prevented. "is_delayed": True, @@ -487,7 +487,7 @@ def test_delay_remove_dataset_job(svc_client_cache, it_remote_repo_url_temp_bran request_payload = { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": "mydata", "migrate_project": True, } @@ -521,7 +521,7 @@ def test_delay_remove_dataset_job_failure(svc_client_cache, it_remote_repo_url_t request_payload = { "git_url": it_remote_repo_url, - "ref": ref, + "branch": ref, "name": dataset_name, } @@ -549,7 +549,7 @@ def test_delay_edit_dataset_job(svc_client_cache, it_remote_repo_url_temp_branch context = DatasetEditRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": "mydata", "title": f"new title => {uuid.uuid4().hex}", # NOTE: We test with this only to check that recursive invocation is being prevented. @@ -588,7 +588,7 @@ def test_delay_edit_dataset_job_failure(svc_client_cache, it_remote_repo_url_tem context = DatasetEditRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": "mydata", "title": f"new title => {uuid.uuid4().hex}", "migrate_project": False, @@ -621,7 +621,7 @@ def test_delay_unlink_dataset_job(svc_client_cache, it_remote_repo_url_temp_bran context = DatasetUnlinkRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": "ds1", "include_filters": ["data1"], # NOTE: We test with this only to check that recursive invocation is being prevented. @@ -658,7 +658,7 @@ def test_delay_unlink_dataset_job_failure(svc_client_cache, it_remote_repo_url_t it_remote_repo_url, branch = it_remote_repo_url_temp_branch context = DatasetUnlinkRequest().load( - {"git_url": it_remote_repo_url, "ref": branch, "name": "ds1", "include_filters": ["data1"]} + {"git_url": it_remote_repo_url, "branch": branch, "name": "ds1", "include_filters": ["data1"]} ) _, _, cache = svc_client_cache @@ -687,7 +687,7 @@ def test_unlink_dataset_sync(svc_client_cache, it_remote_repo_url_temp_branch, v context = DatasetUnlinkRequest().load( { "git_url": it_remote_repo_url, - "ref": branch, + "branch": branch, "name": "ds1", "include_filters": ["data1"], "migrate_project": True, diff --git a/tests/service/jobs/test_project.py b/tests/service/jobs/test_project.py index 4e6c919838..dad1d59fae 100644 --- a/tests/service/jobs/test_project.py +++ b/tests/service/jobs/test_project.py @@ -30,7 +30,9 @@ def test_delay_migration_job(svc_client_cache, it_remote_old_repo_url_temp_branc it_remote_repo_url, branch = it_remote_old_repo_url_temp_branch - context = ProjectMigrateRequest().load({"git_url": it_remote_repo_url, "ref": branch, "skip_docker_update": True}) + context = ProjectMigrateRequest().load( + {"git_url": it_remote_repo_url, "branch": branch, "skip_docker_update": True} + ) _, _, cache = svc_client_cache renku_module = "renku.ui.service.controllers.cache_migrate_project" diff --git a/tests/service/views/test_templates_views.py b/tests/service/views/test_templates_views.py index 52187f0c4a..eeff9937de 100644 --- a/tests/service/views/test_templates_views.py +++ b/tests/service/views/test_templates_views.py @@ -75,7 +75,7 @@ def test_compare_manifests(svc_client_with_templates): assert {"result"} == set(response.json.keys()) assert response.json["result"]["templates"] - templates_source = fetch_templates_source(source=template_params["url"], reference=template_params["ref"]) + templates_source = fetch_templates_source(source=template_params["url"], reference=template_params["branch"]) manifest_file = templates_source.path / TEMPLATE_MANIFEST manifest = TemplatesManifest.from_path(manifest_file).get_raw_content()