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
sudo delete_files not working #47
Comments
Thanks for spotting this Schach, will this soon. |
I was looking at this, and using sudo seems wrong, as it will halt the script and request for sudo password. Patch resolving this: rajeshtaneja@07f5146 Will request Eloy or David to review this before pulling it. |
Thanks @rajeshtaneja, it looks good, I'm just running a test to ensure there are no problems with it's subdirs |
cache/ dir contains subdirectories which are not 777 so they could not be deleted if you remove the sudo, these subdirectories and it's child files are created in produce_page_graph (webapp/lib.php) you could also set it's permissions to 777 so there will be no problems when deleting them without sudo In case you want to test it, all these cache/ files are created when you access the details.php page through the web interface |
Thanks David, Here is a patch fixing all permissions. rajeshtaneja@ab22037 |
Sweet Rajesh, the only issue I can think about is that, even after pulling the patch, if somebody runs clean_data.sh script the previous cache/SUBDIR dirs will remain there as they have the old permissions. Pity that we don't have an upgrade.php, we could chmod 777 -R... up to you. |
Thanks David, Unfortunately this can't be done from shell script, as owner is www-data and not the user executing the script. |
Ok, then we could sudo rm -rf cache instead of sudo delete_files "cache", we would not need webapp/lib.php modifications in that case. |
Hi @rajeshtaneja , I was just looking at the project issues, do you think of the rm -rf cache approach? |
Thanks for looking at this David, this got lost somehow. I still feel we should try avoid sudo where possible, as these scripts are normally used by jobs. IMHO we should fix proper permission and add information about removing old files in update. |
Yeah, I think we should avoid any sudo within any script. If the execution generate files owned by "apache/www", then it will need to be run by "apache/www" or by an user having perms to perform the clean (say, group.. or whatever). Perhaps this is instead about to add, in instructions, the "detail" of which user should execute every script? Note that, perhaps, an alternative would be to make this to be a local_xxx plugin, and to have some php scripts on it, able to be run from the browser, so they will be run with the "correct" user. Although I'm not 100% sold to this idea either. I think I prefer clearer specification, or error message if something was not deleted, saying "are you sure that you ran this with the xxxx user" or so. Just my 2 cents. Ciao :) |
I am not sure about the CLI scripts running by www-data user, the CLI scripts runs by a normal user, privileges over jmeter... and www-data runs only the web app, the issue here is that the web app fills a images cache dir, as it is a cache dir I thought that it should also be cleaned if we have a clean_data.sh script. About local_xxx, would be hard to make it possible as this projects needs full control over an empty clean moodle site, it needs to switch to other branches... IMO given the size of the cache directory contents we could just leave the cache there or create another clean_webapp_data.sh |
Proposal adding a new clean_webapp_data.sh |
I think it is nice solution, but surely would be good to fix permissions in webapp/lib.php |
As commented in #47 (comment) that would not be enough; up to you, whatever you decide I'm fine with it. |
Sure David, I am saying with your patch we should include fix for webapp/lib.php |
We want to clean data from jenkins user setting +w at group it allows us to do it.
We want to clean data from jenkins user setting +w at group it allows us to do it.
We want to clean data from jenkins user setting +w at group it allows us to do it.
As discussed in HQ, I've added 3 branches (master, 28 and 27) including the new script and moving permissions to 775. The new script, run by www-data, deletes cache/ (and if in future there are more temp dirs created by the web app) contents, also the ones created before this patch gets integrated. In our nightly CI server we will:
|
We want to clean data from jenkins user setting +w at group it allows us to do it.
The branches are
|
We want to clean data from jenkins user setting +w at group it allows us to do it.
We want to clean data from jenkins user setting +w at group it allows us to do it.
We want to clean data from jenkins user setting +w at group it allows us to do it.
Thanks David, Branches merged :) |
In clean_data.sh following calls don't work.
sudo delete_files "cache"
sudo delete_files "$backupsdir/*"
Looks like sudoing a bash function is not possible.
Replaced "delete_files" with "rm -rf" and removed the quotes to make it work:
sudo rm -rf cache
sudo rm -rf $backupsdir/*
The text was updated successfully, but these errors were encountered: