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

fix: temporary files not being deleted #969

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

Oppen
Copy link
Contributor

@Oppen Oppen commented Aug 25, 2024

Fixes a path mismatch between a temporary file and its removal.
It also fixes a second removal of the file.

Should fix #966

@Monirzadeh
Copy link
Collaborator

we try to remove os package and handle things with abstract layer can you find why abstract layer not working?

@Oppen
Copy link
Contributor Author

Oppen commented Aug 25, 2024

The reason is the FS domain assumes everything is relative to the data dir, while the temporary file is relative to the root directory. Why are we using the abstraction? Doesn't the os package work well enough?
Fixing it under the abstraction would require bigger changes to the domains module I believe. Not against it but I can't commit to do it myself right now.

Copy link
Member

@fmartingr fmartingr left a comment

Choose a reason for hiding this comment

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

As I said in #966, this should be in in order to fix the problem. i don't think we need any more changes for now in order to work with ephemeral files.

@fmartingr fmartingr requested a review from Monirzadeh August 27, 2024 16:21
@fmartingr fmartingr changed the title fix: lingering tmpfile fix: temporary files not being deleted Aug 27, 2024
Copy link
Collaborator

@Monirzadeh Monirzadeh left a comment

Choose a reason for hiding this comment

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

it is good to have an issue or something in road map to review all the abstraction layer to fix in that level later. @Oppen can you review that when ever you have free time?
any way thanks for investigation and PR.
as i see windows test pass for now so no problem in my side

@Oppen
Copy link
Contributor Author

Oppen commented Aug 27, 2024

it is good to have an issue or something in road map to review all the abstraction layer to fix in that level later. @Oppen can you review that when ever you have free time?

Sure!

any way thanks for investigation and PR. as i see windows test pass for now so no problem in my side
❤️

@fmartingr fmartingr merged commit 2bcb890 into go-shiori:master Aug 28, 2024
9 checks passed
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.

shiori update doesn't clear cache after it's finished, maxing out /tmp/ dir.
3 participants