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

make sure to clean-up depth-first #20

Merged
merged 2 commits into from
Nov 7, 2023
Merged

make sure to clean-up depth-first #20

merged 2 commits into from
Nov 7, 2023

Conversation

b97pla
Copy link
Contributor

@b97pla b97pla commented Nov 3, 2023

What problems does this PR solve?

There was a problem where the service would not properly clean up files and folders not included in the tar archive. This should fix that problem by ensuring that the content is cleaned up depth-first.

Also, the commands are wrapped in bash scripts rather than executed directly. This is not strictly necessary since this was intended to solve an issue with the executing shell but it didn't and therefore another workaround was used. However, it may be useful for troubleshooting to explicitly write the commands to scripts so I'll leave it in for now.

How has the changes been tested?
The unit tests have been run. Ideally, a bit more unit tests could be added but this is not done in this PR.

Reasons for careful code review
If any of the boxes below are checked, extra careful code review should be inititated.

  • This PR contains code that could remove data

wrap commands in script
@b97pla b97pla requested a review from Aratz November 3, 2023 10:43
Copy link

@Aratz Aratz left a comment

Choose a reason for hiding this comment

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

Great work! Looks like this was a tricky problem. I agree there could be more tests (in particular to make sure that the bug we found in staging is solved) but that maybe this time we can let it go.

Had just one comment about adding a couple comments.

archive_upload/handlers/dsmc_handlers.py Show resolved Hide resolved
@b97pla b97pla closed this Nov 6, 2023
@b97pla b97pla reopened this Nov 7, 2023
@b97pla b97pla merged commit a3c5b9f into master Nov 7, 2023
1 check 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.

2 participants