-
Notifications
You must be signed in to change notification settings - Fork 44.6k
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
Please do not 'rm -rf <unquoted variable>. Also please do not 'rm -rf' without confirmation, use 'rm -r' instead! #8056
Conversation
I would be most unhappy if this was removed on my dev machine: $ /usr/bin/du $(poetry env info --path) --max-depth=0 -h|sort -h 14G /home/user/.venv
I would be MOST unhappy if this happened on my dev machine; $ /usr/bin/du $(poetry env info --path) --max-depth=0 -h|sort -h 14G /home/user/.venv
I would be most unhappy if this was deleted on my dev machine: $ /usr/bin/du $(poetry env info --path) --max-depth=0 -h|sort -h 14G /home/user/.venv Also, not quoting a variable fed to "rm -rf" can lead to catastrophy.
PR Reviewer Guide 🔍
|
✅ Deploy Preview for auto-gpt-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #8056 +/- ##
======================================
Coverage ? 57.86%
======================================
Files ? 106
Lines ? 5765
Branches ? 720
======================================
Hits ? 3336
Misses ? 2316
Partials ? 113
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The failing CI test looks like it's unrelated to the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dagelf I'm glad you caught this before you deleted all your packages!
This bit of code is pretty unused now and the tutorials are well out of date. The forge will be removed soon too.
That said, and to avoid us wiping others env without their knowledge, this is a good idea. Thank you 🙏
Forge CI fail seems unrelated
I don't understand, CI issue is unrelated to the functionality this PR introduces |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
I don't have permission to make branches on your fork, and I'm not comfortable updating your master branch to be in line with ours, so I'm going to close this for now, as it's difficult for me to fix any issues that occur. Feel free to update from the master branch then sync branches, and reopen |
Updated #8417 |
Background
I just did the "tutorial" and thankfully I implemented this change before I did, because I would've been MOST unhappy if this happened:
My connectivity isn't that great, and this environment, being my development environment, has packages accumulated over months. This is not my reproducible production machine, it's my personal dev machine... maybe I shouldn't try out new projects outside of a sandbox.... but a lot of people do, and this can really bite them.
Changes 🏗️
Quoted the "$ENV_PATH". Also made sure the script fails, if the rm command fails.
Also added a double confirmation.
Also added a way to skip the confirmation:
touch delete
. We can add that to the docs or any CI, if confirmed safe.This also closes #7404
PR Quality Scorecard ✨
+2 pts
+5 pts
+5 pts
+5 pts
-4 pts
+4 pts
+5 pts
-5 pts
agbenchmark
to verify that these changes do not regress performance?+10 pts