Skip to content
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

Add onedata objectstore #17540

Merged
merged 13 commits into from
May 19, 2024
Merged

Conversation

bwalkowi
Copy link
Contributor

@bwalkowi bwalkowi commented Feb 23, 2024

Object store plugin for Onedata distributed data management system (https://onedata.org/). Requires Onedata version >= 21.02.5.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.

  • This is a refactoring of components with existing test coverage.

  • Instructions for manual testing are as follows:

    1. Add the onedatafilerestclient package to the currently used requirements (can be found in conditional-requirements.txt).

    2. Create object_store_conf.xml file in the config dir, copying the part concerning Onedata from object_store_conf.xml.sample.
      Fill in the config (access_token, onezone_domain, space, disable_tls_certificate_validation - you may want True - see below).

      You will need an account in a Onedata system instance, however, version 21.02.5 is not officially released yet. The best way to test is to use the "demo mode", which allows easily setting up a dockerized Onedata environment: https://onedata.org/#/home/documentation/topic/stable/demo-mode

    3. Create galaxy.yml file in the config dir with the following content:

    galaxy:
      object_store_config_file: object_store_conf.xml
    
    1. Start the galaxy server, log in.

    2. Upload some data and run some jobs. Open the Onedata UI and navigate to your space to see the stored Galaxy datasets.

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@mvdbeek
Copy link
Member

mvdbeek commented Feb 27, 2024

This looks pretty cool, anything we can do to help ?

@bwalkowi
Copy link
Contributor Author

Hello, sorry, I accidentally opened it too early. Although this is a working prototype, we are still working on improvements. In a few days, when the final version is ready, I will reopen the pr.

@bwalkowi bwalkowi reopened this Mar 4, 2024
@bwalkowi
Copy link
Contributor Author

bwalkowi commented Mar 4, 2024

@mvdbeek it's finally ready - I'm reopening the pr.

Comment on lines 140 to 141
log.debug(f"Configuring Onedata connection to {self.onezone_domain} "
f"(disable_tls_certificate_validation={self.disable_tls_certificate_validation})")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (certificate) as clear text.
This expression logs sensitive data (certificate) as clear text.
This expression logs sensitive data (certificate) as clear text.
This expression logs sensitive data (password) as clear text.
This expression logs sensitive data (secret) as clear text.
@jdavcs jdavcs modified the milestones: 24.0, 24.1 Mar 5, 2024
@bwalkowi bwalkowi force-pushed the add-onedata-objectstore branch from be5fcf1 to 85d1cd7 Compare March 6, 2024 09:14
@bwalkowi
Copy link
Contributor Author

bwalkowi commented Mar 6, 2024

I specified the Onedata object store dependencies as optional so that they would not be installed by default and that is why the automated tests failed. Is there a specific requirements.txt file used during testing that I can add them to?

@bgruening
Copy link
Member

@bwalkowi can you please fix the linting errors and update with the main branch. Thanks!

@bwalkowi
Copy link
Contributor Author

bwalkowi commented May 6, 2024

@bgruening done!

lib/galaxy/objectstore/onedata.py Dismissed Show dismissed Hide dismissed
@jmchilton
Copy link
Member

The integration test failures look legitimate.

ObjectStore configured to use Onedata, but no OnedataFileRESTClient dependency available. Please install and properly configure Onedata or modify Object Store configuration.

The conditional dependency handling looks right though - I don't know immediately what to do.

@mvdbeek
Copy link
Member

mvdbeek commented May 9, 2024

Conditional dependencies that we're actually testing need to be added to https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/dependencies/dev-requirements.txt and https://github.com/galaxyproject/galaxy/blob/dev/pyproject.toml#L129

@bwalkowi
Copy link
Contributor Author

nice. It seems everything but code scanning is green. Do you have any recommendations regarding Clear-text logging of sensitive information ? I do not think what is logged is that sensitive and it seems some other files have similar problems.

@mvdbeek
Copy link
Member

mvdbeek commented May 13, 2024

You haven't touched these, so no need to change that here.

@@ -139,6 +139,7 @@ isort = "*"
lxml = "!=4.2.2"
markdown-it-reporter = "*"
myst-parser = "*"
onedatafilerestclient = "==21.2.5rc1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
onedatafilerestclient = "==21.2.5rc1"
onedatafilerestclient = "*"

@mvdbeek mvdbeek requested a review from jmchilton May 13, 2024 07:19
@jmchilton jmchilton merged commit 9d2a45f into galaxyproject:dev May 19, 2024
54 checks passed
Copy link

This PR was merged without a "kind/" label, please correct.

@jmchilton
Copy link
Member

I made a bunch of changes to this with the PR #18174 . By using a new superclass for caching object stores there was a lot less duplication with other object stores and you should have gotten the bug fixes we put into #18117. We merged that PR but I would re-run through it with your setup and let us know if anything broke and send patches. Thanks so much for the contribution!

@bwalkowi
Copy link
Contributor Author

nice! sure thing, I will test it and try to send eventual patches within a week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants