Comment 2 for bug 994699

Revision history for this message
Brandon Holtsclaw (imbrandon) wrote :

Yea , more than fair to question 99% of that , as I knew this was not a complete charm as said before but needed a break from it a few days and an initial review to kick me back off....

ok just wanted to address a couple of points tho before I started jumping back in it, really minor just clarifications to your questions ( minus obvious omissions like maintainer.

Two main points I want to address as they will likely stay in the final charm unless more discussion is had and even then .... anyhow ok ....

>Style questions:
>* Using php, but then not using php?
>run('mkdir -p /mnt/ramdisk/phpfpm-cache');
>run('mkdir -p /mnt/ramdisk/tmp-cache');
>run('mkdir -p /mnt/www');
>Why are you using 'run' instead of just the 'mkdir' function?

I did it like this very deliberately and there is actually a 3rd pattern that you might have overlooked / not added together ( un-documented , but will be when done ) the reasoning of using backtic's as well to run some commands that again may on the surface seem lke using/not using but here is why and its consistent ... php "holds" the output when shelled out and needs to be examined/echo's back out to end up in the juju log, so if you'll notice any commands that may produce output are wrapped in a simple run function that runs the command then echo's as needed ( and can have some additional error checking added as well for exit codes later ) , thus that covers using the run() vs's built-ins for those things, then the backticks are used consistently where return data is NOT expected to goto juju-log and is fed directly to a variable for immediate use ...

ok now for the "templates" if you'll notice those are also very very simple wrapper functions for what equates to a sed replace ( using perl regex instead ) just like many if not every other charm out there, the only difference is i've taken the default config and for values i KNOW i need to replace over and over I put a token in place that is easily picked up my a very simple regex or sed to keep complexity to a minimum. if this charm where in bash i would do identically the same thing ... and as far as complexity I've seen far larger and more complex functions in quite a few charms already so i dont see what keeping them out of it does , other than adding to the workload NOW of putting it into a ppa package then installing it, when yes thats eventually what will happen for the bits that prove themselves but to be honest i dont think the cost / reward is worth doing it at this point for the reasons just given , not to say that it wont ever but working > perfect , especially when every single charm will need to be basically redone for some reason or another more than one time in the next 12 months.

sooooo overall yes i 100% agree with the very small thinks mentioned above ( even if long winded , still simple from both sides )
/me goes to make the changes ...