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

CSV feeds screen broken after deleting the CSV mapper #6757

Closed
SamuelHassine opened this issue Apr 19, 2024 · 5 comments · Fixed by #6891
Closed

CSV feeds screen broken after deleting the CSV mapper #6757

SamuelHassine opened this issue Apr 19, 2024 · 5 comments · Fixed by #6891
Assignees
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)
Milestone

Comments

@SamuelHassine
Copy link
Member

Description

On testing instance: https://testing.octi.staging.filigran.io/dashboard/data/ingestion/csv.

I think, given the errors, is the CSV mapper associated to one of the CSV feed has been deleted.

@SamuelHassine SamuelHassine added bug use for describing something not working as expected needs triage use to identify issue needing triage from Filigran Product team and removed needs triage use to identify issue needing triage from Filigran Product team labels Apr 19, 2024
@SamuelHassine SamuelHassine added this to the Release 6.0.11 milestone Apr 19, 2024
@jborozco jborozco removed the needs triage use to identify issue needing triage from Filigran Product team label Apr 22, 2024
@labo-flg
Copy link
Member

labo-flg commented May 6, 2024

Cascade delete seems to be the issue.

What do we want ?

We can prevent and throw an error when trying to delete a mapper that has ingester(s) using it ; meaning the user has to delete or reconfigure all ingesters before being able to delete the mapper.

OR

We cascade delete : when deleting a mapper all ingesters using it are deleted also. IMO this means we should add a confirmation popup when deleting the mapper, warning that "N ingesters are currently using it, are you sure?".

@labo-flg labo-flg self-assigned this May 6, 2024
@nino-filigran
Copy link

The second option would not prevent the error to happen again right? I mean potentially a user could confirm both pop up and we would still be in the same situation, right? If so, I'm more in favor of the first solution.

@labo-flg
Copy link
Member

labo-flg commented May 6, 2024

The second option would prevent the error because we would delete also all ingesters using this mapper, so the ingestion/feed page would not crash.

@nino-filigran
Copy link

nino-filigran commented May 6, 2024

I think I would still prefer the first option, because if the user has a CSV feed ingester using the specific CSV mapper, the user might delete it which would cause the CSV feed to not work anymore. And I guess we're not letting the user know that something is wrong with a CSV feed.

@labo-flg
Copy link
Member

labo-flg commented May 6, 2024

Ok, agreed.
And we note that managing ingesters and managing csv mappers are under different capabilities, which can be tricky in the equation.

Let's throw an error when deleting a mapper used by one or more ingesters. If we need a more complex approach, let's discuss it in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug use for describing something not working as expected solved use to identify issue that has been solved (must be linked to the solving PR)
Projects
None yet
4 participants