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

Exclude users by Tag name or ID #9

Merged
merged 17 commits into from
May 26, 2023

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented May 22, 2023

Fixes #8.
Use --exclude-tag with Tag ID or Name to exclude members linked to Tag from cleanup

e.g. testing on merge-ci

$ omero demo-cleanup --gigabytes 10 --exclude-tag blah
...
Ignoring users who have logged out within the past 0 days.
Aiming to delete at least 10 bytes of data.
Excluding 2 members linked to Tag:83076 blah
...

cc @pwalczysko @joshmoore

@will-moore
Copy link
Member Author

ERROR: Package 'isort' requires a different Python: 3.6.8 not in '>=3.7.0'

@jburel
Copy link
Member

jburel commented May 23, 2023

ERROR: Package 'isort' requires a different Python: 3.6.8 not in '>=3.7.0'

See #6 (comment)

@will-moore will-moore changed the title Exclude users by group name or ID Exclude users by Tag name or ID May 23, 2023
@pwalczysko
Copy link
Member

Excluding 1 members linked to Tag:125982 waterbottle

@will-moore Looks great, but could I please request a concrete username printout ^^^ ?

@will-moore
Copy link
Member Author

Ooops - I was actually excluding the wrong IDs before (Tag IDs instead of Experimenter IDs).
Fixed now and printing Experimenters:

Excluding 2 members linked to Tag:83076 blah:
  Experimenter:455 user-4 user-4
  Experimenter:3127 3cd6c3bd-8b02 4810-90d7

@pwalczysko
Copy link
Member

Sorry for coming a bit late with this:

I think that the --exclude-tag should be a default behaviour. Just like ignoring the logged-in users is. The day when the runner of this script forgets to use the --exclude-tag flag could be the last day of the data of the persistent users. This has also a further advantage: when this is made a default behaviour, the name of the flag is not so crucial anymore.

@will-moore
Copy link
Member Author

I can make the exclude-tag or ignore-tag into a required argument, so the user has to specify it?

Or I could hard-code a Tag name, e.g. "NO DELETE" and the script would fail if it didn't find that Tag (or if no users were linked to it)? This is a similar approach we use in idr-gallery to find "favourite" images via a hard-coded Tag name.

If you want to allow a user to choose a different ignore-tag then this could be an optional argument, with e.g. "NO DELETE" as the default value? This would allow you to delete the "NO DELETE" users directly (instead of un-tagging them) if desired?

@pwalczysko
Copy link
Member

I can make the exclude-tag or ignore-tag into a required argument, so the user has to specify it?

I like this. How would the user tell the script that there are no users to exclude anymore ? Maybe pass ignore-tag None ?

Or I could hard-code a Tag name, e.g. "NO DELETE" and the script would fail if it didn't find that Tag (or if no users were linked to it)? This is a similar approach we use in idr-gallery to find "favourite" images via a hard-coded Tag name.

NO DELETE could be a tagset and the specific tags under it would have names such as UniversityX ? The usecase here is different than the "favourite" images. The specific tag names would allow more flexibility, and carry some meaning for the user (such as "UniversityX" is not protected anymore, whereas "ProjectK-LabBlah" is the one to be careful of).

If you want to allow a user to choose a different ignore-tag...

Yes, I would like to allow that, but rather as a set of "NO DELETE" tags where the single tag names carry info about "why" to protect these users.

Feel free to simplify if necessary, thanks.

@will-moore
Copy link
Member Author

@pwalczysko That should be working:
Testing with login to merge-ci...
In this case I've used a Tag Group that has 3 child Tags and each are recursively checked for links to Users.
(not limited to 2 levels of hierarchy, which is why each child is also checked for child Tags - could disable that if confusing)?
OR you can use the Tag directly (instead of a Tag Group).

$ omero demo-cleanup --gigabytes 10 --ignore-tag U3TagGroupTest
Ignoring users who have logged out within the past 0 days.
Aiming to delete at least 10 bytes of data.
Tag:90453 U3TagGroupTest has 3 child Tags...
Ignoring 0 members linked to Tag:90454 Sub1:
Tag:90454 Sub1 has 0 child Tags...
Ignoring 0 members linked to Tag:90455 Sub2:
Tag:90455 Sub2 has 0 child Tags...
Ignoring 2 members linked to Tag:83076 blah:
  Experimenter:455 user-4 user-4
  Experimenter:3127 3cd6c3bd-8b02-4810-90d7-de9d9b47efa9 3cd6c3bd-8b02-4810-90d7-de9d9b47efa9
Ignoring "user-3" (#454) who is logged in.
Finding disk usage of "training_user" (#2).
...

@joshmoore
Copy link
Member

Few comments:

  • In general, 👍 for tag ids or value
  • The tag group logic could be complicated, but I assume someone who has taken the time to create a tag group that they put on experiments knows what they are doing.
  • I could see having an --ignore-user flag directly since that logic is available now in the UI.
  • Likely the same could be done for tags on groups, but definitely no rush.

@pwalczysko
Copy link
Member

@joshmoore

  • I could see having an --ignore-user flag directly since that logic is available now in the UI.

Yes, this sounds logical, but I have following problem with it: The usernames are picked by the remote users, not by the runner of the script or by the tagger of the Experimenters. They will not be bearing too much info - very often they are sounding like gibberish to the runner of the script (metadata encoded by a third-party into the name :)). The remote users will also unfortunately create more accounts with similar names, only one of which should be protected. Whereas the Tags are being chosen by the runner of the script or by the tagger, and will more clearly denote the reason for which the particular user is being protected in a way which is understandable by this Team.

@joshmoore
Copy link
Member

For this use case, I agree completely. But for a quick usage: I've run the script in dry-run mode, see that the first to be removed user should be protected (for whatever reason), then I would like to be able to quickly --ignore-user=$USER_ID.

@pwalczysko
Copy link
Member

For this use case, I agree completely. But for a quick usage: I've run the script in dry-run mode, see that the first to be removed user should be protected (for whatever reason), then I would like to be able to quickly --ignore-user=$USER_ID.

Aha, as an additional, optional flag ? I like that... What do you think @will-moore ?

@will-moore
Copy link
Member Author

Makes sense. I guess we should support excluding more than 1 user, and support ID and omeName as IDs?
E.g. --ignore-users 123,user-1,ben,234

@will-moore
Copy link
Member Author

OK added support for --ignore-users 123,user-1,ben,234 and updated README.

@pwalczysko
Copy link
Member

pwalczysko commented May 26, 2023

Happy with this, thank you @will-moore. The main usage I have in mind is in the example below, but also tested the --ignore-users and ignore-tag features, both work as expected. The --ignore-users is an optional flag which consumes the list of usernames or IDs, and the --ignore-tag is a flag which overwrites the default NO DELETE tag or tagset.

(clip36) pwalczysko@Petrs-MacBook-Pro omero-demo-cleanup % omero demo-cleanup --days 30 --gigabytes 100 
Using session for [email protected]:4064. Idle timeout: 10 min. Current group: Lab1
Ignoring users who have logged out within the past 30 days.
Aiming to delete at least 100 bytes of data.
Tag:129695 NO DELETE has 2 child Tags...
Ignoring 1 users linked to Tag:129696 Indonesia-Gadjah-Mada:
  Experimenter:5 Linda Buck
Ignoring 1 users linked to Tag:129697 testing-for-cleanup:
  Experimenter:6 Charles Darwin
Ignoring "trainer-1" (#2) who is logged in.
Ignoring "system-monitoring" (#552) who is logged in.
Ignoring "PUBLIC-USER" (#1902) who logged in recently.
Finding disk usage of "bbecula" (#353).
Finding disk usage of "fm1" (#53).
....

The only request would be: Could I please get the output of the form as below please ? The Ignoring "trainer-1" (#2) ... is listed in the output below to have an example of the pre-existing output form.

...
Ignoring 1 users linked to Tag:129696 Indonesia-Gadjah-Mada:
  "user-2" (#5) Linda Buck
Ignoring 1 users linked to Tag:129697 testing-for-cleanup:
  "user-3" (#6) Charles Darwin
Ignoring "trainer-1" (#2) who is logged in.

This means to get the new output in line with the pre-existing output, and adding the First and Last Name to it, this is:

  1. Do not use the word "Experimenter" -> not much use for it, and it is long and not in line with the pre-existing output
  2. Put the omeName into inverted commas and list it first
  3. Follow the omeName by the user ID in bracket with a hash preceding the user ID
  4. Follow the user ID by the first and last names of the user (this particular point is not in line with the pre-existing output, but I think it is quite useful adition)

@will-moore
Copy link
Member Author

Done

@pwalczysko
Copy link
Member

Done

Looks great, thanks a lot @will-moore

Please if you could merge will-moore#1 ? Just one small typo fix.

After that, ready to merge fmpov.

Remove redundant inverted comma
@will-moore
Copy link
Member Author

Thanks @pwalczysko
@joshmoore - Please merge if you're happy with it?

@joshmoore joshmoore merged commit 6b66f0d into ome:main May 26, 2023
@pwalczysko
Copy link
Member

I can see that deploying this on the demo server would take a change in this line
https://github.com/ome/prod-playbooks/blob/4687065e26c009e2012e0752b2b8bf17f3e4af56/omero/ome-demoserver.yml#L385

Which would assume a new release on PyPI. What would be the steps to achieve that ? I suppose to push a tag here in this repo, and do a release ? Possibly I would just need the permissions on this repo to do that ?

But the PyPi, no idea...

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.

Omit specified users from cleanup
4 participants