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

Set Library Permissions #187

Merged
merged 16 commits into from
Nov 30, 2022

Conversation

mira-miracoli
Copy link
Contributor

I added a tool to set the permissions of ALL datasets in a given data library (in folders and subfolders) at once to the given roles.
It is a workaround for one point of this Issue, I created.

Usage:

usage: set_library_permissions [-h] [-v] [-g GALAXY] [-u USER] [-p PASSWORD] [-a API_KEY] [--library LIBRARY] [--roles ROLES]

Currently the default loglevel is INFO. I don't have experience with this so please tell me if I should change it.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Some small review comments, this will be really nice to have :)

src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

looks great with the new progress bar, fantastic!

@mira-miracoli mira-miracoli requested review from mvdbeek and removed request for hexylena November 21, 2022 14:15
@mira-miracoli mira-miracoli requested review from hexylena and removed request for mvdbeek November 22, 2022 12:14
Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

looks good to me! great work @mira-miracoli

src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
src/ephemeris/set_library_permissions.py Outdated Show resolved Hide resolved
@mira-miracoli
Copy link
Contributor Author

Thank you for your reviews and suggestions @hexylena :)

@hexylena
Copy link
Member

Sorry @mira-miracoli, the linter has some additional requests for you to re-order the imports and add whitespace here and there to make it happy.

@hexylena
Copy link
Member

Traceback (most recent call last):
  File "/home/runner/work/ephemeris/ephemeris/.tox/py36-integration/bin/shed-tools", line 8, in <module>
    sys.exit(main())
 ...
  File "/home/runner/work/ephemeris/ephemeris/.tox/py36-integration/lib/python3.6/site-packages/bioblend/galaxyclient.py", line 266, in key
    headers = self.json_headers.copy()
AttributeError: 'GalaxyInstance' object has no attribute 'json_headers'

that looks unrelated, but not sure what's causing that

@mira-miracoli
Copy link
Contributor Author

I saw it failing in the actions before, I don't know if it is a issue related to my code

@mira-miracoli
Copy link
Contributor Author

Shall I change something or can we merge it?

@hexylena
Copy link
Member

@nsoranzo any ideas about this error?

@nsoranzo
Copy link
Member

That error was fixed in commit galaxyproject/bioblend@22f2d89 , which was first released in BioBlend v0.17.0 . That release also dropped support for Python 3.6 (EOL for almost a year now), which is the version used in the tests.
So my suggestion would be to update the GitHub workflows to use Python 3.7 .

@hexylena hexylena force-pushed the set-library-permissions branch from 9685f9e to 867cf97 Compare November 28, 2022 13:49
@nsoranzo
Copy link
Member

You may also want to add a python_requires (e.g. https://github.com/galaxyproject/gravity/blob/main/setup.py#L39 ) which is metadata used by PyPI and pip.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Someone with more privileges than me (@mvdbeek?) will need to change the branch protection requirements to now depend on the update test names since their name includes the python version.

@natefoo
Copy link
Member

natefoo commented Nov 30, 2022

Someone with more privileges than me (@mvdbeek?) will need to change the branch protection requirements to now depend on the update test names since their name includes the python version.

Got it

@mvdbeek mvdbeek merged commit b2b1cb3 into galaxyproject:master Nov 30, 2022
@mvdbeek
Copy link
Member

mvdbeek commented Nov 30, 2022

Thanks @mira-miracoli and reviewers, very cool work!

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

Successfully merging this pull request may close these issues.

5 participants