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(RepositoryUpdater): make sure temp directories are deleted #2010

Merged

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Mar 2, 2022

Resolves DEV-559

@mpro7 mpro7 requested a review from subotic as a code owner Mar 2, 2022
@mpro7 mpro7 self-assigned this Mar 2, 2022
Copy link
Collaborator

@irinaschubert irinaschubert left a comment

LGTM

@@ -168,14 +191,15 @@ class RepositoryUpdater(
val downloadDir: Path = settings.upgradeDownloadDir match {
case Some(configuredDir) =>
// Yes. Use that directory.
// TODO-mpro never used - see application.conf line 341
Copy link
Collaborator

@irinaschubert irinaschubert Mar 2, 2022

Choose a reason for hiding this comment

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

Can this comment be deleted? The content of application.conf can vary from one setup to another, so the reference to a certain line and value in that conf file may not be valid anymore.

Copy link
Collaborator Author

@mpro7 mpro7 Mar 2, 2022

Choose a reason for hiding this comment

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

You know what, actually you had right. It doesn't make sense., because we never get back to it (at least here).

* Deletes previously created temp directories if these were not deleted after update.
* It could happen if update crashed before ends.
*/
Copy link
Collaborator

@irinaschubert irinaschubert Mar 2, 2022

Choose a reason for hiding this comment

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

In my opinion there is too much logic in this docstring. I would just describe the method with what it is actually doing, something like: "Deletes directories inside temp directory starting with tempDirNamePrefix."

Copy link
Collaborator Author

@mpro7 mpro7 Mar 2, 2022

Choose a reason for hiding this comment

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

True, created docstring before moving filename prefix to the tempDirNamePrefix.

Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

LGTM

subotic
subotic approved these changes Mar 3, 2022
* Deletes directories inside temp directory starting with `tempDirNamePrefix`.
*/
def deleteTempDirectories(): Unit = {
val rootDir = new File("/tmp/")
Copy link
Collaborator

@subotic subotic Mar 3, 2022

Choose a reason for hiding this comment

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

  • You are missing a check for settings.upgradeDownloadDir. If this is set, then the temp directory might not be tmp. It is a valid setting that could be used in production.
  • Also, this will probably not work under Windows. You should use the Paths.get() to have a correct path.

Copy link
Collaborator Author

@mpro7 mpro7 Mar 3, 2022

Choose a reason for hiding this comment

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

I think adding that check doesn't make sense since that part of code is commented out and settings.upgradeDownloadDir in this check will always lead to None option, which goes then to deleteTempDirectories().

Hmmm, is the Paths.get() suggestion because Windows uses backslashes?

Copy link
Collaborator Author

@mpro7 mpro7 Mar 4, 2022

Choose a reason for hiding this comment

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

@subotic I've removed unused code in 881cf90. I don't know why it was commented out, but if my implementation will not work, it can be brought back. That means maybe instead of creating random folders, only one would be used.

I've tried out Paths.get() and Path.of(). Although all compiles, test-repository-upgrade hangs on last script wait-for-knora.sh. Once again, if this implementation somehow fail, idea described above, should make possible to use either Paths.get() or Path.of().

And tmp is default temp folder defined in docker-compose.

Copy link
Collaborator

@subotic subotic Mar 4, 2022

Choose a reason for hiding this comment

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

please don't merge a PR if the discussion is not finished.

The locally defined docker-compose.yml is irrelevant. The app is deployed in production with a different set of settings and another docker-compose.yml. Instead of adding 2 lines of code, you are now risking to break stuff in production. Not the right attitude in my opinion.

Copy link
Collaborator Author

@mpro7 mpro7 Mar 4, 2022

Choose a reason for hiding this comment

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

I'm sorry but thought you are not going to reply and because already accepting this PR there isn't any critical issue here.

If you feel so, this changes could be reverted. But this isn't deployed directly to the production, we have TEST and STAGE too.

Copy link
Collaborator Author

@mpro7 mpro7 Mar 4, 2022

Choose a reason for hiding this comment

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

Just checked PROD configuration and apparently /tmp:/tmp volume is also defined in docker-compose.yml, /tmp/ folder exists and contains knora* update directories to remove.

@sonarcloud
Copy link

@sonarcloud sonarcloud bot commented Mar 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mpro7 mpro7 changed the title fix(RepositoryUpdater): delete temp directories fix(RepositoryUpdater): make sure temp directories are deleted Mar 4, 2022
@mpro7 mpro7 merged commit 9c9a1bd into main Mar 4, 2022
11 checks passed
@mpro7 mpro7 deleted the DEV-559-api-doesnt-cleanup-temp-folder-after-crash-at-startup branch Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants