-
Notifications
You must be signed in to change notification settings - Fork 44.5k
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
ref(classic): Do not 'rm -rf <unquoted variable>' when removing classic env #8417
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-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
✅ 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 #8417 +/- ##
======================================
Coverage ? 58.16%
======================================
Files ? 106
Lines ? 5765
Branches ? 720
======================================
Hits ? 3353
Misses ? 2306
Partials ? 106
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
This is going to be challenging to merge as the CI for the classic is broken since moving it. #8338 |
If it's broken, merging this won't break it further ...... at least people following the instructions linked in the original issue won't be able to accidentally wipe their environments. |
I will look at the CI if I get a chance though...... |
I went and fixed the ci, once its merged #8338, this can go in |
@dagelf I Got this fixed up, so we can merge it! I just need you to sign the CLA :) |
Updated "classic" forge to not accidentally delete my environment without warning. (Updated #8056)
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