1
\$\begingroup\$

At my office we have an internal monitoring tool we use on customer systems to monitor various vitals and notify us via Slack. Our Lead Dev and Chief Architect are very paranoid about Git, and outright refuse to put it on customer systems, however they still like the idea of using it for development.

So I created an autoupdater script for our monitoring tool that allows us to push changes to git, and have the tool auto-update from the repo without having to put git on systems.

It's written in PHP as this is the primary language in use at the office (specifically 5.3.3, although one of our devs is currently writing a technical specification for updating to 5.6), and it uses GitLab's api on our own internal gitlab host.

I've also spoofed a few details for privacy reasons, for example the config file is not called config.conf, and the service is not called tool.

<?php
//Until config can be loaded, assume _LOC is the directory of this script
$_LOC = realpath(dirname(__FILE__)).'/';

//Look for config file
echo "Searching for ".$_LOC."config/config.conf\n";
if (!file_exists($_LOC.'config/config.conf')) {
    echo "Searching for ".$_LOC."config/config.template.conf\n";
    if (!file_exists($_LOC.'config/config.template.conf')) {
        exit('Config file does not exist and template was not found!');
    }

    //Create config from template
    echo "Template exists, creating config file\n";
    copy($_LOC.'config/config.template.conf', $_LOC.'config/config.conf');
}

//Imports
require($_LOC.'lib/functions.php');
require($_LOC.'config/config.conf');
require('/etc/casper/defaults.conf');

if ($_DEBUG) echo "Starting update check\n";

if ($_DEBUG) echo "Fetching commits\n";
//Fetch all commits for set branch, default sorted by newest first
$commitsjson = gitlab_fetch("projects/$_GIT_ID/repository/commits", array('ref' => $_GIT_BRANCH));
$commits = json_decode($commitsjson);
$lastcommit = $commits[0];
$lastcommit = $lastcommit->id;

if ($_DEBUG) echo "Latest commit is ".$lastcommit."\n";
if ($lastcommit !== $_UPDATE_SHA) {
    if ($_DEBUG) echo "But current update is ".$_UPDATE_SHA."\nUpdating...";
    //Fetch recursive tree of all files and directories
    $filejson = gitlab_fetch("projects/$_GIT_ID/repository/tree", array('recursive' => 'true', 'ref' => $_GIT_BRANCH));
    $fileinfo = json_decode($filejson);

    //Create backup directory
    $bakdate = date('Ymd');
    $bakloc = $_LOC.'bak'.$bakdate.'/';
    mkdir($bakloc);

    $files = array();
    $dirs  = array();
    //Format each file to url to fetch file contents and filter directories to separate array
    foreach($fileinfo as $file) {
        if ($file->type == "blob")
            $files[$file->path] = "projects/$_GIT_ID/repository/files/".$file->path;
        else if ($file->type == "tree")
            $dirs[] = $file->path;
    }
    //Create directories
    foreach($dirs as $dir) {
        if ($_DEBUG) echo "Creating directory $dir\n";
        if (!file_exists($_LOC.$dir)) mkdir($_LOC.$dir);
        else mkdir($bakloc.$dir);
    }
    //Fetch files
    foreach($files as $path => $fileapi) {
        if ($_DEBUG) echo "Fetching $path\n";
        //Fetch json
        $fileraw = gitlab_fetch($fileapi, array('ref' => $_GIT_BRANCH));
        $fileobj = json_decode($fileraw);
        //If content is set, file was successfully pulled
        if (isset($fileobj->content)) {
            //content is provided in base64, need to decode
            $filecontent = base64_decode($fileobj->content);
            if ($_DEBUG) echo "Fetched ".strlen($fileobj->content)." bytes, decoded into ".strlen($filecontent)." bytes\n";
            //Backup file if it already exists
            if (file_exists($_LOC.$path)) {
                if ($_DEBUG) echo "Backing up $_LOC$path to $bakloc$path\n";
                copy($_LOC.$path, $bakloc.$path);
            }
            if ($_DEBUG) echo "Overwriting $_LOC$path\n";
            file_put_contents($_LOC.$path, $filecontent);
            if ($_DEBUG) echo "Done\n";
        //If message is set, an error was returned
        } else if (isset($fileobj->message)) {
            //Output the error
            if ($_DEBUG) echo "Failed to download $path with response ".$fileobj->message."\n";
        }
    }

    //Update the commit hash
    if ($_DEBUG) echo "Updating commit hash\n";
    file_put_contents($_LOC.'config/update-hash', $lastcommit);
    if ($_DEBUG) echo "Restarting tool...\n";
    //Log restart in slack, and restart service
    slack_pretty_log('Tool Updated', "Tool $_VERSION has updated! Restarting...", 'good');
    $output = shell_exec('sudo /etc/init.d/tool restart 2>&1');
    if ($_DEBUG) echo $output;
    //End
} else {
    if ($_DEBUG) echo "No new updates\n";
}

Functions such as slack_pretty_log and gitlab_fetch are defined externally and use the respective APIs, and any variables beginning with underscores (such as $_DEBUG and $_GIT_BRANCH) are defined in the config file.

I'm mainly interested in feedback on the flow of the logic itself, as well as naming conventions (which I admittedly didn't do a great job of adhering to) and comments.

This is a standalone script designed to run once per day on a cronjob, which is why it has a lot of logic outside of any functions or classes.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Why have you repeated if ($_DEBUG) echo a zillion times? That is begging to be embedded in a debug() function.

Every time you have a comment with a chunk of logic underneath it, that should be a method (named what the comment says).

\$\endgroup\$
5
  • \$\begingroup\$ I agree with your comment regarding the debug echo repetition, and I've added a debug function for that now, however given the nature of the script, and the nature of function calls in PHP, it seems pointless and even counter productive to needlessly split the entire script up into functions that will only be run once \$\endgroup\$
    – Mayube
    Feb 8, 2018 at 10:17
  • \$\begingroup\$ What, pray tell, is it about functions in PHP that makes introducing them a bad idea? The introduction is for readability, not performance. \$\endgroup\$ Feb 8, 2018 at 17:08
  • \$\begingroup\$ My issue, aside from the performance issues, is that it leads to a code flow like this, which in my opinion worsens readability, rather than improves it \$\endgroup\$
    – Mayube
    Feb 8, 2018 at 17:33
  • \$\begingroup\$ Ah, yes, the performance issues from function overhead on a once-a-day script :) You're also stating, with a straight face, that a nested method call is LESS readable than 20 lines of code nested inside the same if statement? \$\endgroup\$ Feb 8, 2018 at 17:46
  • \$\begingroup\$ Yes I am, because if you just have a series of function calls within an if statement, you essentially have magic black boxes as far as readability is concerned, and you have to scroll down or open a different file to look at the function declarations, which breaks the flow completely \$\endgroup\$
    – Mayube
    Feb 8, 2018 at 21:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.