Installer fine tuning #2993
Installer fine tuning #2993
Conversation
|
Can you let me kmow when you think this is ready for review and merging? I don't want to merge and then have a bunch of follow up merges |
|
Sure, I'll let you know! Updated the title for now... By the way, thanks for this awesome project! |
|
Ok, testing done, think it's ready to get reviewed... [EDIT]: Sorry, just found another hidden issue ;) |
|
@uberbrady can I get a sanity check on this before merge? |
|
is suido a typo on line 19? |
|
Ah, sure...weird. Looks like vim insert typo ;) And fixed... |
| @@ -16,13 +16,12 @@ | |||
|
|
|||
| # ensure running as root | |||
| if [ "$(id -u)" != "0" ]; then | |||
| exec sudo "$0" "$@" | |||
| exec suido "$0" "$@" | |||
There was a problem hiding this comment.
Typo, or is this something I don't know about?
| @@ -36,16 +35,16 @@ spin[1]="\\" | |||
| spin[2]="|" | |||
| spin[3]="/" | |||
|
|
|||
| rm -rf $tmp/ | |||
| rm -rf ${tmp:?} | |||
There was a problem hiding this comment.
Does this mean:
if tmp is defined
`rm -rf tmp`
else
`rm -rf ?`
end
If so, that rm -rf ? will delete any single-character files and recursively delete any single-letter directories. If we're trying to do something to handle when there is no tmp I think I'd prefer something like a straight if statement, or at least rm -rf ${tmp:tmp_directory_is_not_defined_so_this_will_generate_an_error}. I honestly think the if version is the simplest and most importantly, the clearest.
There was a problem hiding this comment.
Not really, this prevents deleting / if $tmp is unset.
https://github.com/koalaman/shellcheck/wiki/SC2115
The colon is bash builtin fallback for unset variables.
| @@ -56,24 +55,26 @@ progress () { | |||
| } | |||
|
|
|||
| vhenvfile () { | |||
| sudo ls -al /etc/apache2/mods-enabled/rewrite.load >> /var/log/snipeit-install.log 2>&1 | |||
| find /etc/apache2/mods-enabled -maxdepth 1 -name 'rewrite.load' >/dev/null 2>&1 | |||
There was a problem hiding this comment.
I think this is fine, it just piques my curiousity - why do this? Is it something about showing up in the logs a little bit clenaer? Well, actually, I guess it won't show up in the logs at all, right?
There was a problem hiding this comment.
| echo " ErrorLog /var/log/apache2/snipeIT.error.log" | ||
| echo " CustomLog /var/log/apache2/access.log combined" | ||
| echo "</VirtualHost>" | ||
| } >> $apachefile |
There was a problem hiding this comment.
much cleaner! I like this.
There was a problem hiding this comment.
😄 Also this: https://github.com/koalaman/shellcheck/wiki/SC2129
| echo >> $hosts "127.0.0.1 $hostname $fqdn" | ||
| a2ensite $name.conf >> /var/log/snipeit-install.log 2>&1 | ||
| a2ensite $name.conf >/dev/null 2>&1 |
There was a problem hiding this comment.
Another one that no longer logs - any particular reason?
There was a problem hiding this comment.
Hm, might be better to log this too...
| @@ -542,6 +556,6 @@ echo "" | |||
| echo "* Cleaning up..." | |||
| rm -f snipeit.sh | |||
| rm -f install.sh | |||
| rm -rf $tmp/ | |||
| rm -rf ${tmp:?} | |||
There was a problem hiding this comment.
That same bit about :? that I mentioned before - sure this is alright?
| rpm -Uvh $tmp/ius-release*.rpm >> /var/log/snipeit-install.log 2>&1 | ||
| yum -y install wget epel-release >/dev/null 2>&1 | ||
| wget -P $tmp/ https://centos7.iuscommunity.org/ius-release.rpm >/dev/null 2>&1 | ||
| rpm -Uvh $tmp/ius-release*.rpm >/dev/null 2>&1 |
There was a problem hiding this comment.
No more logging in here?
| echo -n " ##" $p "Installing... " | ||
| yum -y install $p >> /var/log/snipeit-install.log 2>&1 | ||
| echo -n " ## installing $p ... " | ||
| yum -y install "$p" >/dev/null 2>&1 |
| wget -P $tmp/ https://github.com/snipe/snipe-it/archive/$file >> /var/log/snipeit-install.log 2>&1 | ||
| unzip -qo $tmp/$file -d $tmp/ | ||
| cp -R $tmp/$fileName $webdir/$name | ||
| wget -P "$tmp/" "https://github.com/snipe/snipe-it/archive/$file" >/dev/null 2>&1 |
| sudo chmod -R 755 $webdir/$name/storage/private_uploads | ||
| sudo chmod -R 755 $webdir/$name/public/uploads | ||
| sudo chown -R apache:apache $webdir/$name | ||
| perms |
There was a problem hiding this comment.
Ah, there's that "perms" thing again. What's that?
log()to get rid of duplicate code (adding results to logfile)Tested on Debian / Ubuntu using Docker