From 46d10df87014d8b7ea3c2f78f7f71bd8760515d6 Mon Sep 17 00:00:00 2001 From: Alexandru Mahmoud Date: Thu, 16 Sep 2021 17:01:51 -0400 Subject: [PATCH 01/16] Add image building workflow Remove 'release_' from branch names Change branch name Add quay Quay login --- .github/workflows/build_image.yaml | 45 ++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 .github/workflows/build_image.yaml diff --git a/.github/workflows/build_image.yaml b/.github/workflows/build_image.yaml new file mode 100644 index 00000000000..47dab58409d --- /dev/null +++ b/.github/workflows/build_image.yaml @@ -0,0 +1,45 @@ +name: Build Image +on: push +jobs: + test: + name: Build image + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + # https://stackoverflow.com/questions/59810838/how-to-get-the-short-sha-for-the-github-workflow + - name: Set outputs + id: vars + run: echo "::set-output name=sha_short::$(git rev-parse --short HEAD)" + - name: Set branch name + id: branch + run: echo "::set-output name=name::$(BRANCH_NAME=${GITHUB_REF##*/}; echo ${BRANCH_NAME/release_/}-auto)" + - name: Login to docker hub + uses: actions-hub/docker/login@master + env: + DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }} + DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} + - run: docker build . -t galaxy/galaxy-min:${{ steps.vars.outputs.sha_short }} -f .k8s_ci.Dockerfile + - name: Push to docker hub with commit ID + uses: actions-hub/docker@master + with: + args: push galaxy/galaxy-min:${{ steps.vars.outputs.sha_short }} + - run: docker tag galaxy/galaxy-min:${{ steps.vars.outputs.sha_short }} galaxy/galaxy-min:${{ steps.branch.outputs.name }} + - name: Push to docker hub with branch name + uses: actions-hub/docker@master + with: + args: push galaxy/galaxy-min:${{ steps.branch.outputs.name }} + - run: docker tag galaxy/galaxy-min:${{ steps.vars.outputs.sha_short }} quay.io/galaxy-k8s/galaxy:${{ steps.branch.outputs.name }} && docker tag galaxy/galaxy-min:${{ steps.vars.outputs.sha_short }} quay.io/galaxy-k8s/galaxy:${{ steps.vars.outputs.sha_short }} + - name: Login to docker hub + uses: actions-hub/docker/login@master + env: + DOCKER_USERNAME: ${{ secrets.QUAY_USERNAME }} + DOCKER_PASSWORD: ${{ secrets.QUAY_PASSWORD }} + DOCKER_REGISTRY_URL: quay.io + - name: Push to quay.io with commit ID + uses: actions-hub/docker@master + with: + args: push quay.io/galaxy-k8s/galaxy:${{ steps.vars.outputs.sha_short }} + - name: Push to quay.io with branch name + uses: actions-hub/docker@master + with: + args: push quay.io/galaxy-k8s/galaxy:${{ steps.branch.outputs.name }} From 3925c17026bbbe1237dd94ed515c1e7d31a8e46d Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 29 Oct 2021 08:53:16 +0200 Subject: [PATCH 02/16] Fix wrong dataset details link Fixes https://github.com/galaxyproject/galaxy/issues/12804. This is not the ideal fix, the client should ideally contruct the link ... but it's the old history, it's an easy fix and it's consistent with what the new history does (for now). There is some backbone manipulation of the model id (which is not HistoryDatasetAssociation, but a DatasetCollectionElement in a collection context) which is supposed to set the id to the HistoryDatasetAssociation id. I guess it may not have run on time ? I also don't know where it is and how to find it, might have been deleted ? I think it was related to https://github.com/galaxyproject/galaxy/blob/dev/client/src/mvc/collection/collection-model.js#L86. --- client/src/mvc/dataset/dataset-li.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/client/src/mvc/dataset/dataset-li.js b/client/src/mvc/dataset/dataset-li.js index c91444f1c4d..ba0e646aa3c 100644 --- a/client/src/mvc/dataset/dataset-li.js +++ b/client/src/mvc/dataset/dataset-li.js @@ -9,7 +9,6 @@ import BASE_MVC from "mvc/base-mvc"; import _l from "utils/localization"; import { mountNametags } from "components/Nametags"; import { Toast } from "ui/toast"; -import { getAppRoot } from "onload/loadConfig"; var logNamespace = "dataset"; /*============================================================================== @@ -273,8 +272,6 @@ export var DatasetListItemView = _super.extend( * @returns {jQuery} rendered DOM */ _renderShowParamsButton: function () { - const url = `datasets/${this.model.get("id")}/details`; - return faIconButton({ title: _l("View details"), classes: "params-btn", @@ -286,12 +283,12 @@ export var DatasetListItemView = _super.extend( if (Galaxy.frame && Galaxy.frame.active) { ev.preventDefault(); Galaxy.frame.add({ - url: `${getAppRoot()}${url}`, + url: this.model.urls.show_params, title: `Dataset Details of ${this.model.get("name")}`, }); } else if (Galaxy.router) { ev.preventDefault(); - Galaxy.router.push(url); + Galaxy.router.push(this.model.urls.show_params); Galaxy.trigger("activate-hda", this.model.get("id")); } }, From 6d1edce5255ae9898fba6d1b06f44cacd4b7702f Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 29 Oct 2021 14:23:35 +0200 Subject: [PATCH 03/16] If datatype is unknown display warning when trying to connect nodes --- client/src/components/Workflow/Editor/modules/terminals.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/client/src/components/Workflow/Editor/modules/terminals.js b/client/src/components/Workflow/Editor/modules/terminals.js index e7b1a79b82a..5a19f927a7a 100644 --- a/client/src/components/Workflow/Editor/modules/terminals.js +++ b/client/src/components/Workflow/Editor/modules/terminals.js @@ -340,6 +340,11 @@ class BaseInputTerminal extends Terminal { this._isSubType(cat_outputs[other_datatype_i], thisDatatype) ) { return new ConnectionAcceptable(true, null); + } else if (!this.datatypesMapper.datatypes.includes(other_datatype)) { + return new ConnectionAcceptable( + false, + `Effective output data type [${other_datatype}] unknown. This tool cannot be executed on this Galaxy Server at this moment, please contact the Administrator.` + ); } } } From 513f694f6afd522c7d32931d9730ea12c2683121 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 30 Oct 2021 09:36:04 +0200 Subject: [PATCH 04/16] Restore ToolEntryPoints component Fixes https://github.com/galaxyproject/galaxy/issues/12692 --- client/src/components/Tool/ToolForm.vue | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/src/components/Tool/ToolForm.vue b/client/src/components/Tool/ToolForm.vue index 0a1ad81b0bb..879369f44e4 100644 --- a/client/src/components/Tool/ToolForm.vue +++ b/client/src/components/Tool/ToolForm.vue @@ -102,6 +102,7 @@ import ConfigProvider from "components/providers/ConfigProvider"; import LoadingSpan from "components/LoadingSpan"; import FormDisplay from "components/Form/FormDisplay"; import FormElement from "components/Form/FormElement"; +import ToolEntryPoints from "components/ToolEntryPoints/ToolEntryPoints"; import ToolSuccess from "./ToolSuccess"; import UserHistories from "components/History/providers/UserHistories"; import Webhook from "components/Common/Webhook"; @@ -115,6 +116,7 @@ export default { FormDisplay, ToolCard, FormElement, + ToolEntryPoints, ToolSuccess, UserHistories, Webhook, From 8507756ae41d5927da244d27946d67131a9bd190 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 30 Oct 2021 16:24:01 +0200 Subject: [PATCH 05/16] Refactor SiwftObjectStore test so it can be re-used more easily --- test/integration/objectstore/_base.py | 85 +++++++++++++++++++ .../objectstore/test_swift_objectstore.py | 82 +----------------- 2 files changed, 88 insertions(+), 79 deletions(-) diff --git a/test/integration/objectstore/_base.py b/test/integration/objectstore/_base.py index be1df67f4c2..734d6f3780c 100644 --- a/test/integration/objectstore/_base.py +++ b/test/integration/objectstore/_base.py @@ -1,5 +1,7 @@ import os import re +import string +import subprocess from galaxy_test.base.populators import ( DatasetPopulator, @@ -7,6 +9,46 @@ from galaxy_test.base.populators import ( from galaxy_test.driver import integration_util +OBJECT_STORE_HOST = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_HOST', '127.0.0.1') +OBJECT_STORE_PORT = int(os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_PORT', 9000)) +OBJECT_STORE_ACCESS_KEY = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_ACCESS_KEY', 'minioadmin') +OBJECT_STORE_SECRET_KEY = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_SECRET_KEY', 'minioadmin') +OBJECT_STORE_CONFIG = string.Template(""" + + + + + + + + + + + + +""") + + +def start_minio(container_name): + minio_start_args = [ + 'docker', + 'run', + '-p', + f'{OBJECT_STORE_PORT}:9000', + '-d', + '--name', + container_name, + '--rm', + 'minio/minio:latest', + 'server', + '/tmp/data'] + subprocess.check_call(minio_start_args) + + +def stop_minio(container_name): + subprocess.check_call(['docker', 'rm', '-f', container_name]) + + class BaseObjectStoreIntegrationTestCase(integration_util.IntegrationTestCase): framework_tool_and_types = True @@ -34,3 +76,46 @@ class BaseObjectStoreIntegrationTestCase(integration_util.IntegrationTestCase): def files_count(directory): return sum(len(files) for _, _, files in os.walk(directory)) + + +@integration_util.skip_unless_docker() +class BaseSwiftObjectStoreIntegrationTestCase(BaseObjectStoreIntegrationTestCase): + + @classmethod + def setUpClass(cls): + cls.container_name = "%s_container" % cls.__name__ + start_minio(cls.container_name) + super().setUpClass() + + @classmethod + def tearDownClass(cls): + stop_minio(cls.container_name) + super().tearDownClass() + + @classmethod + def handle_galaxy_config_kwds(cls, config): + temp_directory = cls._test_driver.mkdtemp() + cls.object_stores_parent = temp_directory + cls.object_store_cache_path = f"{temp_directory}/object_store_cache" + config_path = os.path.join(temp_directory, "object_store_conf.xml") + config["object_store_store_by"] = "uuid" + config["metadata_strategy"] = "extended" + config["outpus_to_working_dir"] = True + config["retry_metadata_internally"] = False + with open(config_path, "w") as f: + f.write( + OBJECT_STORE_CONFIG.safe_substitute( + { + "temp_directory": temp_directory, + "host": OBJECT_STORE_HOST, + "port": OBJECT_STORE_PORT, + "access_key": OBJECT_STORE_ACCESS_KEY, + "secret_key": OBJECT_STORE_SECRET_KEY, + } + ) + ) + config["object_store_config_file"] = config_path + + def setUp(self): + super().setUp() + self.dataset_populator = DatasetPopulator(self.galaxy_interactor) diff --git a/test/integration/objectstore/test_swift_objectstore.py b/test/integration/objectstore/test_swift_objectstore.py index 6ab1d1c0dae..f7f698af80a 100644 --- a/test/integration/objectstore/test_swift_objectstore.py +++ b/test/integration/objectstore/test_swift_objectstore.py @@ -1,27 +1,6 @@ -import os -import string -import subprocess - from galaxy_test.driver import integration_util +from ._base import BaseSwiftObjectStoreIntegrationTestCase -OBJECT_STORE_HOST = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_HOST', '127.0.0.1') -OBJECT_STORE_PORT = int(os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_PORT', 9000)) -OBJECT_STORE_ACCESS_KEY = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_ACCESS_KEY', 'minioadmin') -OBJECT_STORE_SECRET_KEY = os.environ.get('GALAXY_INTEGRATION_OBJECT_STORE_SECRET_KEY', 'minioadmin') -OBJECT_STORE_CONFIG = string.Template(""" - - - - - - - - - - - - -""") TEST_TOOL_IDS = [ "multi_output", "multi_output_configured", @@ -48,64 +27,9 @@ TEST_TOOL_IDS = [ ] -def start_minio(container_name): - minio_start_args = [ - 'docker', - 'run', - '-p', - f'{OBJECT_STORE_PORT}:9000', - '-d', - '--name', - container_name, - # '--rm', - 'minio/minio:latest', - 'server', - '/tmp/data'] - subprocess.check_call(minio_start_args) - - -def stop_minio(container_name): - subprocess.check_call(['docker', 'rm', '-f', container_name]) - - -@integration_util.skip_unless_docker() -class SwiftObjectStoreIntegrationTestCase(integration_util.IntegrationTestCase): - - @classmethod - def setUpClass(cls): - cls.container_name = "%s_container" % cls.__name__ - start_minio(cls.container_name) - super().setUpClass() - - @classmethod - def tearDownClass(cls): - stop_minio(cls.container_name) - super().tearDownClass() - - @classmethod - def handle_galaxy_config_kwds(cls, config): - temp_directory = cls._test_driver.mkdtemp() - cls.object_stores_parent = temp_directory - config_path = os.path.join(temp_directory, "object_store_conf.xml") - config["object_store_store_by"] = "uuid" - config["metadata_strategy"] = "extended" - config["outpus_to_working_dir"] = True - config["retry_metadata_internally"] = False - with open(config_path, "w") as f: - f.write( - OBJECT_STORE_CONFIG.safe_substitute( - { - "temp_directory": temp_directory, - "host": OBJECT_STORE_HOST, - "port": OBJECT_STORE_PORT, - "access_key": OBJECT_STORE_ACCESS_KEY, - "secret_key": OBJECT_STORE_SECRET_KEY, - } - ) - ) - config["object_store_config_file"] = config_path +class SwiftObjectStoreIntegrationTestCase(BaseSwiftObjectStoreIntegrationTestCase): + pass instance = integration_util.integration_module_instance(SwiftObjectStoreIntegrationTestCase) - test_tools = integration_util.integration_tool_runner(TEST_TOOL_IDS) From 673e068358d1a170fe3c1642fe80c1b8f8b82241 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 30 Oct 2021 16:24:36 +0200 Subject: [PATCH 06/16] Add test case that verifies various object store cache operations --- lib/galaxy_test/base/populators.py | 4 +- ...est_remote_objectstore_cache_operations.py | 48 +++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 test/integration/objectstore/test_remote_objectstore_cache_operations.py diff --git a/lib/galaxy_test/base/populators.py b/lib/galaxy_test/base/populators.py index ec6f71eb9cb..3252c77516c 100644 --- a/lib/galaxy_test/base/populators.py +++ b/lib/galaxy_test/base/populators.py @@ -337,8 +337,8 @@ class BaseDatasetPopulator(BasePopulator): delete_response = self._delete(f"histories/{history_id}") delete_response.raise_for_status() - def delete_dataset(self, history_id: str, content_id: str) -> Response: - delete_response = self._delete(f"histories/{history_id}/contents/{content_id}") + def delete_dataset(self, history_id: str, content_id: str, purge: bool = False) -> Response: + delete_response = self._delete(f"histories/{history_id}/contents/{content_id}", {'purge': purge}) return delete_response def create_tool_from_path(self, tool_path: str) -> Response: diff --git a/test/integration/objectstore/test_remote_objectstore_cache_operations.py b/test/integration/objectstore/test_remote_objectstore_cache_operations.py new file mode 100644 index 00000000000..f922989370d --- /dev/null +++ b/test/integration/objectstore/test_remote_objectstore_cache_operations.py @@ -0,0 +1,48 @@ +import os +import shutil + +import pytest + +from ._base import ( + BaseSwiftObjectStoreIntegrationTestCase, + files_count, +) + + +class CacheOperationTestCase(BaseSwiftObjectStoreIntegrationTestCase): + + def tearDown(self): + shutil.rmtree(self.object_store_cache_path) + os.mkdir(self.object_store_cache_path) + return super().tearDown() + + def upload_dataset(self): + history_id = self.dataset_populator.new_history() + hda = self.dataset_populator.new_dataset(history_id, content='123', wait=True) + return hda + + def test_cache_populated(self): + self.upload_dataset() + assert files_count(self.object_store_cache_path) == 1 + + def test_cache_repopulated(self): + hda = self.upload_dataset() + assert files_count(self.object_store_cache_path) == 1 + shutil.rmtree(self.object_store_cache_path) + os.mkdir(self.object_store_cache_path) + assert files_count(self.object_store_cache_path) == 0 + content = self.dataset_populator.get_history_dataset_content(hda['history_id'], content_id=hda['id']) + assert content == '123\n' + assert files_count(self.object_store_cache_path) == 1 + + def test_delete_item_not_in_cache(self): + hda = self.upload_dataset() + assert files_count(self.object_store_cache_path) == 1 + shutil.rmtree(self.object_store_cache_path) + os.mkdir(self.object_store_cache_path) + assert files_count(self.object_store_cache_path) == 0 + self.dataset_populator.delete_dataset(hda['history_id'], hda['id'], purge=True) + # Don't wait for dataset, this uses the history state, which is new if there is no dataset ... + with pytest.raises(AssertionError) as excinfo: + self.dataset_populator.get_history_dataset_content(hda['history_id'], content_id=hda['id'], wait=False, assert_ok=False) + assert "File Not Found" in str(excinfo.value) From 49025893dbb7dea3cd5f495136e1e2357d5e8f90 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 30 Oct 2021 16:26:05 +0200 Subject: [PATCH 07/16] Never fail to delete remote object store files if cache path doesn't exist We correctly realized this is a problem in the irods object store, but never applied this to the other object stores. Fixes https://github.com/galaxyproject/galaxy/issues/11940 --- lib/galaxy/objectstore/azure_blob.py | 7 ++++--- lib/galaxy/objectstore/cloud.py | 5 +++-- lib/galaxy/objectstore/irods.py | 15 ++++++++------- lib/galaxy/objectstore/s3.py | 6 ++++-- lib/galaxy/util/__init__.py | 11 +++++++++++ test/unit/objectstore/test_objectstore.py | 23 +++++++++++++++++++++-- 6 files changed, 51 insertions(+), 16 deletions(-) diff --git a/lib/galaxy/objectstore/azure_blob.py b/lib/galaxy/objectstore/azure_blob.py index 24a8754a666..51405dc4fa7 100644 --- a/lib/galaxy/objectstore/azure_blob.py +++ b/lib/galaxy/objectstore/azure_blob.py @@ -23,7 +23,8 @@ from galaxy.exceptions import ( ) from galaxy.util import ( directory_hash_id, - umask_fix_perms + umask_fix_perms, + unlink, ) from galaxy.util.path import safe_relpath from galaxy.util.sleeper import Sleeper @@ -428,7 +429,7 @@ class AzureBlobObjectStore(ConcreteObjectStore): # with all the files in it. This is easy for the local file system, # but requires iterating through each individual blob in Azure and deleing it. if entire_dir and extra_dir: - shutil.rmtree(self._get_cache_path(rel_path)) + shutil.rmtree(self._get_cache_path(rel_path), ignore_errors=True) blobs = self.service.list_blobs(self.container_name, prefix=rel_path) for blob in blobs: log.debug("Deleting from Azure: %s", blob) @@ -436,7 +437,7 @@ class AzureBlobObjectStore(ConcreteObjectStore): return True else: # Delete from cache first - os.unlink(self._get_cache_path(rel_path)) + unlink(self._get_cache_path(rel_path), ignore_errors=True) # Delete from S3 as well if self._in_azure(rel_path): log.debug("Deleting from Azure: %s", rel_path) diff --git a/lib/galaxy/objectstore/cloud.py b/lib/galaxy/objectstore/cloud.py index f7ee33baf89..f53efbfcefa 100644 --- a/lib/galaxy/objectstore/cloud.py +++ b/lib/galaxy/objectstore/cloud.py @@ -17,6 +17,7 @@ from galaxy.util import ( directory_hash_id, safe_relpath, umask_fix_perms, + unlink, ) from galaxy.util.sleeper import Sleeper from .s3 import parse_config_xml @@ -608,7 +609,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): # with all the files in it. This is easy for the local file system, # but requires iterating through each individual key in S3 and deleing it. if entire_dir and extra_dir: - shutil.rmtree(self._get_cache_path(rel_path)) + shutil.rmtree(self._get_cache_path(rel_path), ignore_errors=True) results = self.bucket.objects.list(prefix=rel_path) for key in results: log.debug("Deleting key %s", key.name) @@ -616,7 +617,7 @@ class Cloud(ConcreteObjectStore, CloudConfigMixin): return True else: # Delete from cache first - os.unlink(self._get_cache_path(rel_path)) + unlink(self._get_cache_path(rel_path), ignore_errors=True) # Delete from S3 as well if self._key_exists(rel_path): key = self.bucket.objects.get(rel_path) diff --git a/lib/galaxy/objectstore/irods.py b/lib/galaxy/objectstore/irods.py index 9c504eafd82..eb6841f6c51 100644 --- a/lib/galaxy/objectstore/irods.py +++ b/lib/galaxy/objectstore/irods.py @@ -18,7 +18,12 @@ except ImportError: irods = None from galaxy.exceptions import ObjectInvalid, ObjectNotFound -from galaxy.util import directory_hash_id, ExecutionTimer, umask_fix_perms +from galaxy.util import ( + directory_hash_id, + ExecutionTimer, + umask_fix_perms, + unlink, +) from galaxy.util.path import safe_relpath from ..objectstore import DiskObjectStore @@ -544,7 +549,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): # with all the files in it. This is easy for the local file system, # but requires iterating through each individual key in irods and deleing it. if entire_dir and extra_dir: - shutil.rmtree(self._get_cache_path(rel_path)) + shutil.rmtree(self._get_cache_path(rel_path), ignore_errors=True) col_path = f"{self.home}/{str(rel_path)}" col = None @@ -568,11 +573,7 @@ class IRODSObjectStore(DiskObjectStore, CloudConfigMixin): else: # Delete from cache first - try: - os.unlink(self._get_cache_path(rel_path)) - except FileNotFoundError: - # File was not in cache. Ok to ignore the exception and move on - pass + unlink(self._get_cache_path(rel_path), ignore_errors=True) # Delete from irods as well p = Path(rel_path) data_object_name = p.stem + p.suffix diff --git a/lib/galaxy/objectstore/s3.py b/lib/galaxy/objectstore/s3.py index 4b739af6430..0f7079fe78f 100644 --- a/lib/galaxy/objectstore/s3.py +++ b/lib/galaxy/objectstore/s3.py @@ -24,6 +24,7 @@ from galaxy.util import ( directory_hash_id, string_as_bool, umask_fix_perms, + unlink, which, ) from galaxy.util.path import safe_relpath @@ -492,6 +493,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): rel_path, source_file) except S3ResponseError: log.exception("Trouble pushing S3 key '%s' from file '%s'", rel_path, source_file) + raise return False def file_ready(self, obj, **kwargs): @@ -612,7 +614,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): # with all the files in it. This is easy for the local file system, # but requires iterating through each individual key in S3 and deleing it. if entire_dir and extra_dir: - shutil.rmtree(self._get_cache_path(rel_path)) + shutil.rmtree(self._get_cache_path(rel_path), ignore_errors=True) results = self._bucket.get_all_keys(prefix=rel_path) for key in results: log.debug("Deleting key %s", key.name) @@ -620,7 +622,7 @@ class S3ObjectStore(ConcreteObjectStore, CloudConfigMixin): return True else: # Delete from cache first - os.unlink(self._get_cache_path(rel_path)) + unlink(self._get_cache_path(rel_path), ignore_errors=True) # Delete from S3 as well if self._key_exists(rel_path): key = Key(self._bucket, rel_path) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index fd4d2e56f55..250229e6ff2 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -1503,6 +1503,17 @@ def force_symlink(source, link_name): raise e +def unlink(path_or_fd, ignore_errors=False): + """Calls os.unlink on `path_or_fd`, and ignore FileNoteFoundError if ignore_errors is True.""" + try: + os.unlink(path_or_fd) + except FileNotFoundError: + if ignore_errors: + pass + else: + raise + + def move_merge(source, target): # when using shutil and moving a directory, if the target exists, # then the directory is placed inside of it diff --git a/test/unit/objectstore/test_objectstore.py b/test/unit/objectstore/test_objectstore.py index 06c15114d54..13d7104d748 100644 --- a/test/unit/objectstore/test_objectstore.py +++ b/test/unit/objectstore/test_objectstore.py @@ -1,13 +1,21 @@ import os -from tempfile import mkdtemp +from tempfile import ( + mkdtemp, + mkstemp, +) from uuid import uuid4 +import pytest + from galaxy.exceptions import ObjectInvalid from galaxy.objectstore.azure_blob import AzureBlobObjectStore from galaxy.objectstore.cloud import Cloud from galaxy.objectstore.pithos import PithosObjectStore from galaxy.objectstore.s3 import S3ObjectStore -from galaxy.util import directory_hash_id +from galaxy.util import ( + directory_hash_id, + unlink, +) from ..unittest_utils.objectstore_helpers import ( Config as TestConfig, DISK_TEST_CONFIG, @@ -15,6 +23,17 @@ from ..unittest_utils.objectstore_helpers import ( ) +def test_unlink_path(): + with pytest.raises(FileNotFoundError): + unlink(uuid4().hex) + unlink(uuid4().hex, ignore_errors=True) + fd, path = mkstemp() + os.close(fd) + assert os.path.exists(path) + unlink(path) + assert not os.path.exists(path) + + def test_disk_store(): for config_str in [DISK_TEST_CONFIG, DISK_TEST_CONFIG_YAML]: with TestConfig(config_str) as (directory, object_store): From ca4a2d728d84a734230c2d902e6b7c4ac8ebb617 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 30 Oct 2021 16:32:51 +0200 Subject: [PATCH 08/16] Drop heuristic that was supposed to prevent moving empty working directory outputs This broke updating remote object stores from their cache directory, as evidenced by a broken test_cache_repopulated test. --- lib/galaxy/metadata/set_metadata.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/galaxy/metadata/set_metadata.py b/lib/galaxy/metadata/set_metadata.py index 30489fa029a..fae175bf38a 100644 --- a/lib/galaxy/metadata/set_metadata.py +++ b/lib/galaxy/metadata/set_metadata.py @@ -300,10 +300,9 @@ def set_metadata_portable(): dataset.state = dataset.dataset.state = final_job_state if extended_metadata_collection: - outputs_to_working = external_filename.startswith(tool_job_working_directory) and os.path.getsize(external_filename) - if not link_data_only and outputs_to_working: - # outputs to working directory, and not already pushed by pulsar + extended metadata, - # move output to final destination. + if not link_data_only and os.path.getsize(external_filename): + # Here we might be updating a disk based objectstore when outputs_to_working_directory is used, + # or a remote object store from its cache path. object_store.update_from_file(dataset.dataset, file_name=external_filename, create=True) # TODO: merge expression_context into tool_provided_metadata so we don't have to special case this (here and in _finish_dataset) meta = tool_provided_metadata.get_dataset_meta(output_name, dataset.dataset.id, dataset.dataset.uuid) From 12a4b2e9d1bebb392e6100af54034282a344b629 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 31 Oct 2021 07:18:20 +0100 Subject: [PATCH 09/16] Ignore SameFileError --- lib/galaxy/objectstore/__init__.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py index c20044e9399..14638c30526 100644 --- a/lib/galaxy/objectstore/__init__.py +++ b/lib/galaxy/objectstore/__init__.py @@ -625,6 +625,10 @@ class DiskObjectStore(ConcreteObjectStore): path = self._get_filename(obj, **kwargs) shutil.copy(file_name, path) umask_fix_perms(path, self.config.umask, 0o666) + except shutil.SameFileError: + # That's ok, we need to ignore this so that remote object stores can update + # the remote object from the cache file path + pass except OSError as ex: log.critical(f'Error copying {file_name} to {self.__get_filename(obj, **kwargs)}: {ex}') raise ex From 1a531cd1a034724e809744f38c5c46c9308e2cd2 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 31 Oct 2021 12:19:58 +0100 Subject: [PATCH 10/16] Fix from work dir glob with extended metadata I also have a commit coming for pulsar that fixes this if pulsar stages out from_work_dir outputs. --- lib/galaxy/metadata/set_metadata.py | 13 +++++++++++- test/functional/tools/from_work_dir_glob.xml | 20 +++++++++++++++++++ test/functional/tools/samples_tool_conf.xml | 1 + .../test_pulsar_embedded_extended_metadata.py | 1 + 4 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 test/functional/tools/from_work_dir_glob.xml diff --git a/lib/galaxy/metadata/set_metadata.py b/lib/galaxy/metadata/set_metadata.py index 30489fa029a..11589ee2c2a 100644 --- a/lib/galaxy/metadata/set_metadata.py +++ b/lib/galaxy/metadata/set_metadata.py @@ -10,6 +10,7 @@ set to the path of the dataset on which metadata is being set (output_filename_override could previously be left empty and the path would be constructed automatically). """ +import glob import json import logging import os @@ -55,7 +56,10 @@ from galaxy.tool_util.parser.stdio import ( ToolStdioRegex, ) from galaxy.tool_util.provided_metadata import parse_tool_provided_metadata -from galaxy.util import stringify_dictionary_keys +from galaxy.util import ( + safe_contains, + stringify_dictionary_keys, +) from galaxy.util.expressions import ExpressionContext logging.basicConfig() @@ -270,6 +274,13 @@ def set_metadata_portable(): set_meta_kwds = stringify_dictionary_keys(json.load(open(filename_kwds))) # load kwds; need to ensure our keywords are not unicode try: external_filename = unnamed_id_to_path.get(dataset_instance_id, dataset_filename_override) + if not os.path.exists(external_filename): + matches = glob.glob(external_filename) + assert len(matches) == 1, f"More than one file matched by output glob '{external_filename}'" + external_filename = matches[0] + assert safe_contains(tool_job_working_directory, external_filename), f"Cannot collect output '{external_filename}' from outside of working directory" + created_from_basename = os.path.relpath(external_filename, os.path.join(tool_job_working_directory, 'working')) + dataset.dataset.created_from_basename = created_from_basename # override filename if we're dealing with outputs to working directory and dataset is not linked to link_data_only = metadata_params.get("link_data_only") if not link_data_only: diff --git a/test/functional/tools/from_work_dir_glob.xml b/test/functional/tools/from_work_dir_glob.xml new file mode 100644 index 00000000000..dddb4c97205 --- /dev/null +++ b/test/functional/tools/from_work_dir_glob.xml @@ -0,0 +1,20 @@ + + output1.txt + ]]> + + + + + + + + + + + + + + + + diff --git a/test/functional/tools/samples_tool_conf.xml b/test/functional/tools/samples_tool_conf.xml index 0f9ad2f292a..bcf4de8167a 100644 --- a/test/functional/tools/samples_tool_conf.xml +++ b/test/functional/tools/samples_tool_conf.xml @@ -251,6 +251,7 @@ + diff --git a/test/integration/test_pulsar_embedded_extended_metadata.py b/test/integration/test_pulsar_embedded_extended_metadata.py index 17cd3c02e5b..2be7482a154 100644 --- a/test/integration/test_pulsar_embedded_extended_metadata.py +++ b/test/integration/test_pulsar_embedded_extended_metadata.py @@ -28,5 +28,6 @@ test_tools = integration_util.integration_tool_runner( "simple_constructs", "metadata_bam", # "job_properties", # https://github.com/galaxyproject/galaxy/issues/11813 + "from_work_dir_glob" ] ) From c27c86d27595f08060accb1a513c8e3234c12a57 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 31 Oct 2021 14:19:38 +0100 Subject: [PATCH 11/16] Disable created_from_basename test, doesn't work with default setup --- test/functional/tools/from_work_dir_glob.xml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/functional/tools/from_work_dir_glob.xml b/test/functional/tools/from_work_dir_glob.xml index dddb4c97205..0fb29b57e15 100644 --- a/test/functional/tools/from_work_dir_glob.xml +++ b/test/functional/tools/from_work_dir_glob.xml @@ -13,7 +13,9 @@ echo "hi" > output1.txt - + From 0d93169c1f911d229db3e1c4df7ffbe19e51c8bc Mon Sep 17 00:00:00 2001 From: Assunta DeSanto Date: Fri, 15 Oct 2021 17:36:58 -0400 Subject: [PATCH 12/16] increased matchPercentage used for autopairing --- .../src/components/Collections/PairedListCollectionCreator.vue | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/Collections/PairedListCollectionCreator.vue b/client/src/components/Collections/PairedListCollectionCreator.vue index e87b91cfb68..6e5d953c205 100644 --- a/client/src/components/Collections/PairedListCollectionCreator.vue +++ b/client/src/components/Collections/PairedListCollectionCreator.vue @@ -480,7 +480,7 @@ export default { filters: this.DEFAULT_FILTERS, automaticallyPair: true, initialPairsPossible: true, - matchPercentage: 0.9, + matchPercentage: 0.99, twoPassAutoPairing: true, removeExtensions: true, workingElements: [], From 63e25b20f4e18fc76fe4baeb4c25c221c42a961c Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 31 Oct 2021 16:27:51 +0100 Subject: [PATCH 13/16] Set profile version of ncbi_datasets so it can run containerized --- tools/data_source/ncbi_datasets.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/data_source/ncbi_datasets.xml b/tools/data_source/ncbi_datasets.xml index 1af074b07ee..ebe19c0dac3 100644 --- a/tools/data_source/ncbi_datasets.xml +++ b/tools/data_source/ncbi_datasets.xml @@ -1,4 +1,4 @@ - + import data from the NCBI Datasets Genomes page operation_0224 From 28000701d795354eb5e4cf3041abd057678aad60 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 31 Oct 2021 15:41:40 +0100 Subject: [PATCH 14/16] Allow dataset / collection deletion for anonymous users Fixes https://github.com/galaxyproject/galaxy/issues/12442 --- lib/galaxy/tool_util/verify/interactor.py | 8 +++- .../webapps/galaxy/api/history_contents.py | 3 +- lib/galaxy_test/api/test_history_contents.py | 17 +++++++++ lib/galaxy_test/api/test_jobs.py | 37 ++++++++----------- lib/galaxy_test/base/api.py | 14 ++++++- 5 files changed, 52 insertions(+), 27 deletions(-) diff --git a/lib/galaxy/tool_util/verify/interactor.py b/lib/galaxy/tool_util/verify/interactor.py index 7b9cacca7b7..eb2d5320ada 100644 --- a/lib/galaxy/tool_util/verify/interactor.py +++ b/lib/galaxy/tool_util/verify/interactor.py @@ -90,6 +90,7 @@ class GalaxyInteractorApi: def __init__(self, **kwds): self.api_url = f"{kwds['galaxy_url'].rstrip('/')}/api" + self.cookies = None self.master_api_key = kwds["master_api_key"] self.api_key = self.__get_user_key(kwds.get("api_key"), kwds.get("master_api_key"), test_user=kwds.get("test_user")) if kwds.get('user_api_key_is_admin_key', False): @@ -728,8 +729,11 @@ class GalaxyInteractorApi: def _get(self, path, data=None, key=None, headers=None, admin=False, anon=False): headers = self.api_key_header(key=key, admin=admin, anon=anon, headers=headers) url = self.get_api_url(path) + kwargs = {} + if self.cookies: + kwargs['cookies'] = self.cookies # no data for GET - return requests.get(url, params=data, headers=headers, timeout=util.DEFAULT_SOCKET_TIMEOUT) + return requests.get(url, params=data, headers=headers, timeout=util.DEFAULT_SOCKET_TIMEOUT, **kwargs) def get_api_url(self, path: str) -> str: if path.startswith("http"): @@ -782,6 +786,8 @@ class GalaxyInteractorApi: else: data.update(params) kwd['data'] = data + if self.cookies: + kwd['cookies'] = self.cookies return kwd diff --git a/lib/galaxy/webapps/galaxy/api/history_contents.py b/lib/galaxy/webapps/galaxy/api/history_contents.py index b7c4370d376..6a49c36d29a 100644 --- a/lib/galaxy/webapps/galaxy/api/history_contents.py +++ b/lib/galaxy/webapps/galaxy/api/history_contents.py @@ -1686,8 +1686,7 @@ class HistoryContentsController(BaseGalaxyAPIController, UsesLibraryMixinItems, """ return self.service.validate(trans, history_id, history_content_id) - # TODO: allow anonymous del/purge and test security on this - @expose_api + @expose_api_anonymous def delete(self, trans, history_id, id, purge=False, recursive=False, **kwd): """ DELETE /api/histories/{history_id}/contents/{id} diff --git a/lib/galaxy_test/api/test_history_contents.py b/lib/galaxy_test/api/test_history_contents.py index 0f14a881afc..2fc5d8c8b95 100644 --- a/lib/galaxy_test/api/test_history_contents.py +++ b/lib/galaxy_test/api/test_history_contents.py @@ -290,6 +290,23 @@ class HistoryContentsApiTestCase(ApiTestCase): assert delete_response.status_code < 300 # Something in the 200s :). assert str(self.__show(hda1).json()["deleted"]).lower() == "true" + def test_delete_anon(self): + with self._different_user(anon=True): + history_id = self.dataset_populator.new_history() + hda1 = self.dataset_populator.new_dataset(history_id) + self.dataset_populator.wait_for_history(history_id) + assert str(self.__show(hda1).json()["deleted"]).lower() == "false" + delete_response = self._delete(f"histories/{history_id}/contents/{hda1['id']}") + assert delete_response.status_code < 300 # Something in the 200s :). + assert str(self.__show(hda1).json()["deleted"]).lower() == "true" + + def test_delete_permission_denied(self): + hda1 = self.dataset_populator.new_dataset(self.history_id) + with self._different_user(anon=True): + delete_response = self._delete(f"histories/{self.history_id}/contents/{hda1['id']}") + assert delete_response.status_code == 403 + assert delete_response.json()['err_msg'] == 'HistoryDatasetAssociation is not owned by user' + def test_purge(self): hda1 = self.dataset_populator.new_dataset(self.history_id) self.dataset_populator.wait_for_history(self.history_id) diff --git a/lib/galaxy_test/api/test_jobs.py b/lib/galaxy_test/api/test_jobs.py index 998f3ead0c2..fc6c39cebb2 100644 --- a/lib/galaxy_test/api/test_jobs.py +++ b/lib/galaxy_test/api/test_jobs.py @@ -6,7 +6,6 @@ from operator import itemgetter import requests -from galaxy.util import DEFAULT_SOCKET_TIMEOUT from galaxy_test.api.test_tools import TestsTools from galaxy_test.base.api_asserts import assert_status_code_is_ok from galaxy_test.base.populators import ( @@ -322,32 +321,26 @@ steps: @skip_without_tool('detect_errors_aggressive') def test_report_error(self): with self.dataset_populator.test_history() as history_id: - payload = self.dataset_populator.run_tool_payload( - tool_id='detect_errors_aggressive', - inputs={'error_bool': 'true'}, - history_id=history_id, - ) - run_response = self._post("tools", data=payload).json() - job_id = run_response['jobs'][0]["id"] - self.dataset_populator.wait_for_job(job_id) - dataset_id = run_response['outputs'][0]['id'] - response = self._post(f'jobs/{job_id}/error', - data={'dataset_id': dataset_id}) - assert response.status_code == 200, response.text + self._run_error_report(history_id) @skip_without_tool('detect_errors_aggressive') def test_report_error_anon(self): - # Need to get a cookie and use that for anonymous tool runs - cookies = requests.get(self.url, timeout=DEFAULT_SOCKET_TIMEOUT).cookies - payload = json.dumps({"tool_id": "detect_errors_aggressive", - "inputs": {"error_bool": "true"}}) - run_response = requests.post(f"{self.galaxy_interactor.api_url}/tools", data=payload, cookies=cookies, timeout=DEFAULT_SOCKET_TIMEOUT).json() + with self._different_user(anon=True): + history_id = self.dataset_populator.new_history() + self._run_error_report(history_id) + + def _run_error_report(self, history_id): + payload = self.dataset_populator.run_tool_payload( + tool_id='detect_errors_aggressive', + inputs={'error_bool': 'true'}, + history_id=history_id, + ) + run_response = self._post("tools", data=payload).json() job_id = run_response['jobs'][0]["id"] + self.dataset_populator.wait_for_job(job_id) dataset_id = run_response['outputs'][0]['id'] - response = requests.post(f'{self.galaxy_interactor.api_url}/jobs/{job_id}/error', - data={'email': 'someone@domain.com', 'dataset_id': dataset_id}, - cookies=cookies, - timeout=DEFAULT_SOCKET_TIMEOUT) + response = self._post(f'jobs/{job_id}/error', + data={'dataset_id': dataset_id}) assert response.status_code == 200, response.text @skip_without_tool('detect_errors_aggressive') diff --git a/lib/galaxy_test/base/api.py b/lib/galaxy_test/base/api.py index 0bc30bc2604..2e6107d55ef 100644 --- a/lib/galaxy_test/base/api.py +++ b/lib/galaxy_test/base/api.py @@ -2,6 +2,8 @@ import os from contextlib import contextmanager from urllib.parse import urlencode +import requests + from .api_asserts import ( assert_error_code_is, assert_has_keys, @@ -59,7 +61,7 @@ class UsesApiTestCaseMixin: return user, self._post(f"users/{user['id']}/api_key", admin=True).json() @contextmanager - def _different_user(self, email=OTHER_USER): + def _different_user(self, email=OTHER_USER, anon=False): """ Use in test cases to switch get/post operations to act as new user ..code-block:: python @@ -70,7 +72,13 @@ class UsesApiTestCaseMixin: """ original_api_key = self.user_api_key original_interactor_key = self.galaxy_interactor.api_key - user, new_key = self._setup_user_get_key(email) + original_cookies = self.galaxy_interactor.cookies + if anon: + cookies = requests.get(self.url).cookies + self.galaxy_interactor.cookies = cookies + new_key = None + else: + _, new_key = self._setup_user_get_key(email) try: self.user_api_key = new_key self.galaxy_interactor.api_key = new_key @@ -78,6 +86,7 @@ class UsesApiTestCaseMixin: finally: self.user_api_key = original_api_key self.galaxy_interactor.api_key = original_interactor_key + self.galaxy_interactor.cookies = original_cookies def _get(self, *args, **kwds): return self.galaxy_interactor.get(*args, **kwds) @@ -121,6 +130,7 @@ class ApiTestInteractor(BaseInteractor): """ def __init__(self, test_case, api_key=None): + self.cookies = None admin = getattr(test_case, "require_admin_user", False) test_user = TEST_USER if not admin else ADMIN_TEST_USER super().__init__(test_case, test_user=test_user, api_key=api_key) From 1a968952cc001c4fa1413f3f8ff982276ba64202 Mon Sep 17 00:00:00 2001 From: Simon Bray Date: Mon, 1 Nov 2021 11:57:48 +0100 Subject: [PATCH 15/16] add missing tool_ids to workflow invocation failure message --- lib/galaxy/webapps/galaxy/api/workflows.py | 5 +++-- lib/galaxy_test/api/test_workflows.py | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/webapps/galaxy/api/workflows.py b/lib/galaxy/webapps/galaxy/api/workflows.py index e958fb0213c..73c27bdc60c 100644 --- a/lib/galaxy/webapps/galaxy/api/workflows.py +++ b/lib/galaxy/webapps/galaxy/api/workflows.py @@ -838,8 +838,9 @@ class WorkflowsAPIController(BaseGalaxyAPIController, UsesStoredWorkflowMixin, U raise exceptions.RequestParameterInvalidException("Must specify 'batch' to use batch parameters.") tool_ids = self.workflow_contents_manager.get_all_tool_ids(workflow) - if not all([self.app.toolbox.has_tool(tool_id) for tool_id in tool_ids]): - raise exceptions.MessageException("Workflow was not invoked; some required tools are not installed.") + missing_tool_ids = [tool_id for tool_id in tool_ids if not self.app.toolbox.has_tool(tool_id)] + if missing_tool_ids: + raise exceptions.MessageException(f"Workflow was not invoked; the following required tools are not installed: {', '.join(missing_tool_ids)}") invocations = [] for run_config in run_configs: diff --git a/lib/galaxy_test/api/test_workflows.py b/lib/galaxy_test/api/test_workflows.py index 9a84a5d0351..ae0b636aecf 100644 --- a/lib/galaxy_test/api/test_workflows.py +++ b/lib/galaxy_test/api/test_workflows.py @@ -939,7 +939,7 @@ steps: """) invocation_response = self.__invoke_workflow(history_id, workflow_id, assert_ok=False) self._assert_status_code_is(invocation_response, 400) - self.assertEqual(invocation_response.json().get('err_msg'), "Workflow was not invoked; some required tools are not installed.") + self.assertEqual(invocation_response.json().get('err_msg'), "Workflow was not invoked; the following required tools are not installed: nonexistent_tool") @skip_without_tool("collection_creates_pair") def test_workflow_run_output_collections(self): From aa7b10e8f8f639225705a822e578de4b1302fd6e Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Mon, 1 Nov 2021 13:11:29 +0100 Subject: [PATCH 16/16] Only build docker image for galaxyproject repo limit concurrency and only build for release and anvil branches. --- ...ild_image.yaml => build_container_image.yaml} | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) rename .github/workflows/{build_image.yaml => build_container_image.yaml} (89%) diff --git a/.github/workflows/build_image.yaml b/.github/workflows/build_container_image.yaml similarity index 89% rename from .github/workflows/build_image.yaml rename to .github/workflows/build_container_image.yaml index 47dab58409d..3c11ea3f064 100644 --- a/.github/workflows/build_image.yaml +++ b/.github/workflows/build_container_image.yaml @@ -1,9 +1,17 @@ -name: Build Image -on: push +name: Build Container Image +on: + push: + branches: + - 'release*' + - anvil +concurrency: + group: docker-build-${{ github.ref }} + cancel-in-progress: true jobs: - test: - name: Build image + build: + name: Build container image runs-on: ubuntu-latest + if: github.repository_owner == 'galaxyproject' steps: - uses: actions/checkout@v2 # https://stackoverflow.com/questions/59810838/how-to-get-the-short-sha-for-the-github-workflow