-
Notifications
You must be signed in to change notification settings - Fork 2
fix: sync search data when groups or projects are removed #1075
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
base: main
Are you sure you want to change the base?
Conversation
|
You can access the deployment of this PR at https://renku-ci-ds-1075.dev.renku.ch |
When deleting a project, contained data connectors should be deleted as well. This is currently handled by a trigger in the db watching the `entity_slugs` table.
Removes the trigger that did this before and implements it into the corresponding repositories
b3693f8 to
d2a5da4
Compare
Pull Request Test Coverage Report for Build 18750422297Details
💛 - Coveralls |
| projs = await session.execute( | ||
| select(distinct(schemas.EntitySlugORM.project_id)) | ||
| .join(schemas.NamespaceORM, schemas.NamespaceORM.id == schemas.EntitySlugORM.namespace_id) | ||
| .where(schemas.NamespaceORM.group_id == group.id) | ||
| .where(schemas.EntitySlugORM.project_id.is_not(None)) | ||
| ) | ||
| projs = [e for e in projs.scalars().all() if e] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, this is too many projects at the moment, see this comment: #1004 (comment)
If there was any data connector in a project that was moved out, then there is a remaining entry in the EntitySlugORM table which will match that moved project and the project will get wrongly deleted.
We can look at merging this PR now or decide to fix the slug table issue first. Not sure what is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, great you spotted this! Unfortunate, but I probably would fix the other issue first or maybe disable the "move a project" feature for the next release so things get not messed up even more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution looks good to me 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an empty file would be fine. "Only here to make pytest work." is not correct anyway, __init__.py files are required to make a folder a Python module, so almost all folders with Python source code would have a __init__.py file in them, except for folders containing Python scripts or other types of non-module cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks for the explanation. For me it is confusing that pytest works when I give it the test_xyz.py file, but it doesn't work when it tries to collect test cases. So it is sort of correct to say it makes pytest work :-) I'll remove the comment.
When projects or groups are removed, the containing entities are removed as well. This wasn't propagated to the search engine. Deleting users does not apply, because this is not implemented yet (in case they have data).
/deploy