Delete the contents of the /tmp directory #76

edruder · 2024-03-16T16:49:55Z

Cached files in the /tmp directory can cause errors after renaming the project--nuke them so they don't cause grief.

nickjj

Thanks, I've dropped in a few comments.

nickjj · 2024-03-16T16:55:57Z

bin/rename-project

+The contents of the /tmp directory need to be deleted because they contain
+references to the old project name and can cause errors if they're not removed.
+
+Some of the files to be deleted are owned by users that only exist in the


This project will run your container as a non-root user, files that land in tmp/ will be owned by the user of the process. We shouldn't need to ever use sudo here.

Hmm. I only added sudo because without it I got error messages about needing sudo.

I'll give it a shot.

Can you show the output of ls -la tmp/ when it produced an error?

Now I don't see any error! ¯\(ツ)/¯

I didn't preserve that session, so OOL.

I wonder if you ran any Docker commands with sudo. That would have an effect.

No, I don't use sudo unless forced. 🙂

nickjj · 2024-03-16T16:56:47Z

bin/rename-project

+# the `.keep` file.
+# -----------------------------------------------------------------------------
+function delete_tmp {
+  sudo rm -rf ./tmp/* && touch ./tmp/.keep


Going back to message in discussion #75, can you address this?

I'd suggest recursively deleting all of the contents in the tmp directory except for the .keep file since tmp/.keep is committed to version control. This will leave the original modified times on the directory and keep file too.

The touched tmp/.keep does have a different modified time, but it doesn't show up as a modified file in git.

Do you think the modified time matters, since the modified time of the rest of the files are either the time when the repo was cloned, or the time when the rename script was run?

I think if we leave the tmp/ directory in tact with the original modified time of the file, it's worth doing the same for the keep file. Something like find tmp/ ! -name ".keep" -delete should do the trick but I didn't test it. We would want to make sure the solution works with both the macOS and GNU version of find.

nickjj · 2024-03-16T16:58:16Z

bin/rename-project

+  sudo rm -rf ./tmp/* && touch ./tmp/.keep
+}
+
+while true; do


I think we can get by without needing a prompt for this.

nickjj · 2024-03-16T17:00:53Z

bin/rename-project

@@ -51,6 +51,38 @@ find . -type f -exec \
    -pe "s/${FIND_MODULE_NAME}/${MODULE_NAME}/g;" {} +
 # -----------------------------------------------------------------------------

+cat << 'EOF'


I think all of this can go away and you can drop a 1 liner to delete the contents of the directory in this section of the script The core of the script which renames a few things. and then adjust this comment to be something like The core of the script.

Given this rename script is likely used shortly after creating the project and temporary files are temporary, there's nothing of value to protect or warn against.

edruder · 2024-03-16T18:08:07Z

I just pushed changes that I think simplify things the way that you want, though I haven't changed to a find that doesn't delete the .keep file.

edruder · 2024-03-16T19:16:58Z

I just pushed changes that use find s.t. the .keep file isn't deleted.
I changed your find suggestion slightly s.t. it works on both MacOS and Ubuntu.
I tweaked the comment s.t. it explains the need to delete the contents of the tmp directory.

nickjj · 2024-03-17T01:23:22Z

I changed your find suggestion slightly s.t. it works on both MacOS and Ubuntu.

Do you have a source around needing to set mindepth for it to work on macOS? I don't have a Mac to test.

I tweaked the comment s.t. it explains the need to delete the contents of the tmp directory.

I think we can remove the comments all together and adjust it to: #76 (comment)

Other than these 2 things, you can delete this text from the readme file too:

If you get an error upping the project related to RuntimeError: invalid bytecode then you have old tmp/ files sitting around related to the old project name, you can run ./run clean to clear all temporary files and fix the error.

edruder · 2024-03-18T22:40:13Z

I changed your find suggestion slightly s.t. it works on both MacOS and Ubuntu.

Do you have a source around needing to set mindepth for it to work on macOS? I don't have a Mac to test.

macOS didn't need mindepth–it worked fine with your exact suggestion. When I tried the same command on an Ubuntu VPS that I have, it didn't work–I found mindepth in an example somewhere, tried it, and it worked on my Ubuntu VPS. Then tried the command with mindepth on macOS and it still worked. WDYT?

I tweaked the comment s.t. it explains the need to delete the contents of the tmp directory.

I think we can remove the comments all together and adjust it to: #76 (comment)

Other than these 2 things, you can delete this text from the readme file too:

If you get an error upping the project related to RuntimeError: invalid bytecode then you have old tmp/ files sitting around related to the old project name, you can run ./run clean to clear all temporary files and fix the error.

OK. I'll modify the PR to reflect this.

nickjj · 2024-03-18T23:13:45Z

Sounds good, we can keep it in. I made a test directory and filled it with touch .keep && mkdir -p ok/cool && touch ok/cool/yep && touch sure. Both commands left a .keep file but without -mindepth 1, it threw a non-empty directory error.

bin/rename-project

nickjj

Beyond the review comment, can you also update the changelog file?

You can add the following bullet under A whole bunch of changes related to Rails 7.1.0 which is under the unreleased changes:

Update rename-project script to auto-delete temporary files

Then it's good to go for being merged.

JBerendes · 2024-03-19T15:40:21Z

I don't seem to have any problems with anything else in the tmp except the server.pid not getting cleaned up on a crash or computer restart.

I added this function and haven't had issues
function dc_up { rm -f tmp/pids/server.pid docker compose up "${@}" }

now i use
./run dc_up or ./run dc_up --build

Cached files in the /tmp directory can cause errors after renaming the project--nuke them so they don't cause grief.

nickjj · 2024-03-19T17:33:09Z

Yep that's what I noticed too in practice but other changes can trigger it, #16 has more details.

For pid files, there's #65, there's a few ways to solve this such as configuring Puma and other services not to write pid files or cleaning them up automatically in an entrypoint.

nickjj

Looks good, thanks a lot for powering through this!

nickjj requested changes Mar 16, 2024

View reviewed changes

edruder force-pushed the edruder/delete-tmp-dir branch from 74be6ff to 3dc988f Compare March 16, 2024 18:02

edruder force-pushed the edruder/delete-tmp-dir branch from 3dc988f to d0bc06e Compare March 16, 2024 19:13

edruder force-pushed the edruder/delete-tmp-dir branch from d0bc06e to e21d804 Compare March 18, 2024 23:15

edruder requested a review from nickjj March 18, 2024 23:16

nickjj reviewed Mar 18, 2024

View reviewed changes

bin/rename-project Outdated Show resolved Hide resolved

nickjj requested changes Mar 18, 2024

View reviewed changes

Delete the contents of the /tmp directory

4336f30

Cached files in the /tmp directory can cause errors after renaming the project--nuke them so they don't cause grief.

edruder force-pushed the edruder/delete-tmp-dir branch from e21d804 to 4336f30 Compare March 19, 2024 16:09

edruder requested a review from nickjj March 19, 2024 20:38

nickjj approved these changes Mar 19, 2024

View reviewed changes

nickjj merged commit 89514db into nickjj:main Mar 19, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete the contents of the /tmp directory #76

Delete the contents of the /tmp directory #76

edruder commented Mar 16, 2024

nickjj left a comment

nickjj Mar 16, 2024

edruder Mar 16, 2024

nickjj Mar 16, 2024 •

edited

edruder Mar 16, 2024 •

edited

nickjj Mar 16, 2024

edruder Mar 16, 2024

nickjj Mar 16, 2024

edruder Mar 16, 2024

nickjj Mar 16, 2024

nickjj Mar 16, 2024

nickjj Mar 16, 2024

edruder commented Mar 16, 2024

edruder commented Mar 16, 2024 •

edited

nickjj commented Mar 17, 2024 •

edited

edruder commented Mar 18, 2024

nickjj commented Mar 18, 2024 •

edited

nickjj left a comment

JBerendes commented Mar 19, 2024

nickjj commented Mar 19, 2024

nickjj left a comment

Delete the contents of the /tmp directory #76

Delete the contents of the /tmp directory #76

Conversation

edruder commented Mar 16, 2024

nickjj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickjj Mar 16, 2024 • edited

Choose a reason for hiding this comment

edruder Mar 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edruder commented Mar 16, 2024

edruder commented Mar 16, 2024 • edited

nickjj commented Mar 17, 2024 • edited

edruder commented Mar 18, 2024

nickjj commented Mar 18, 2024 • edited

nickjj left a comment

Choose a reason for hiding this comment

JBerendes commented Mar 19, 2024

nickjj commented Mar 19, 2024

nickjj left a comment

Choose a reason for hiding this comment

nickjj Mar 16, 2024 •

edited

edruder Mar 16, 2024 •

edited

edruder commented Mar 16, 2024 •

edited

nickjj commented Mar 17, 2024 •

edited

nickjj commented Mar 18, 2024 •

edited