fix(RepositoryUpdater): make sure temp directories are deleted #2010
Conversation
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. | ||
| */ |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
True, created docstring before moving filename prefix to the tempDirNamePrefix.
| * Deletes directories inside temp directory starting with `tempDirNamePrefix`. | ||
| */ | ||
| def deleteTempDirectories(): Unit = { | ||
| val rootDir = new File("/tmp/") |
There was a problem hiding this comment.
- You are missing a check for
settings.upgradeDownloadDir. If this is set, then the temp directory might not betmp. 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Kudos, SonarCloud Quality Gate passed!
|
mpro7
changed the title
mpro7
deleted the
DEV-559-api-doesnt-cleanup-temp-folder-after-crash-at-startup
branch
Resolves DEV-559