[colug-432] First script - would anyone care to review?

Fri Jun 5 00:01:43 EDT 2015

Thank you both so much!

I can tell you really spent some time going through everything. I'm going to implement many of your suggestions.

Today I added this script into a kickstart file and used a here doc to create the two .ldif files. I will work back through my use of single and double quotes and back ticks as well as define some variables up front.

Thanks again for the review!

> On Jun 4, 2015, at 11:09 PM, Rick Hornsby <richardjhornsby at gmail.com> wrote:
> 
> 
> 
> Stephen Potter wrote:
>> Zach-
>> Overall, this looks like fairly clean code.
> Most of my comments are around improvements, rather than any fatal 
> issues.  I explain _why_ I point out certain improvements, and I want to 
> be clear - those reasons should not be taken as a reflection on or a 
> personal criticism of you.
> 
> For a first script, I'm impressed for the same reason - you've obviously 
> taken care and pain to write readable code and provide plenty of 
> comments.  Keep doing that.  I don't see anything that's too insane or 
> crazy or that I'd give you a bad grade over.  It is nicely formatted, 
> most of your indenting is where it should be and not all crazy jacked up.
> 
> Most of your certutil and pk12util commands are called out with a short 
> description.  Keep doing this.  You will be glad you did, and so will 
> those who read your code long after you abandon it.
> 
> +1 for avoiding the useless use of cat.
>> I do, however, see a lot of
>> code duplication that could lead to typos or forgetting to update a
>> particular section during script maintenance.  For example, not that
>> this currently affects the script, look at the difference between lines
>> 25, 39, and 46.  You have used double quotes in one case and single
>> quotes in the others.
> Be careful with your quoting, especially in shell scripts.  double quote 
> and single quote can have very different, and mutually exclusive, 
> consequences.
> 
> There is an alternative syntax to the backticks that I encourage my 
> engineers to use because I find it is much easier to read.  These two 
> are functionally equivalent:
> 
> $ipaddr=`ip addr`
> 
> $ipaddr=$(ip addr)
> 
> The backticks can be hard to tell apart from the single quote 
> character.  In the end, this is a personal choice.  I see both in your 
> script.  Try not to mix and match the styles.
> 
> 
> nit: $ipadrss -> prefer the more common usage $ipaddr
> 
> 
>>   From an efficiency standpoint, I also see several places where you run
>> the same command multiple times to get different information each time,
>> where you might be able to tighten it up and only do it once.  However,
>> in this day and age with the speed of systems, this kind of efficiency
>> may no longer be something to be concerned about.
> If it isn't necessary to ask the system to do the same thing multiple 
> times, it's just generally better not to.  Duplicating a command does go 
> to maintainability and readability.  Use variables instead of repeating 
> the same string/path/command over and over.  This is more true for 
> custom paths than standard or reasonably well-known ones, but could be 
> applied to both.
> 
> "Use variables" applies not just to the result of commands, but to 
> things that you're using multiple times as well, like the path
> 
>     /root/ldapca
> 
> I see this path repeated many times.  Set it once somewhere near the top 
> and use the variable for the remainder.  You will be much happier when 
> you need to change that path or reference it again.  By using the 
> variable, you help ensure that you don't accidentally forget an 's' in 
> cacpassword in one place, or change the path half-way through and not 
> realize it.  For example:
> 
>     ldapca_path=/root/ldapca
>     capassword_file=$ldapca_path/capassword
> 
> (I use _path and _file here because I'm trying to reduce ambiguity of 
> the variable name.  Is 'capassword' a string representing a password?  
> No, in this case it represents a file so I call it out in the variable 
> name.)
> 
> Using the variable also helps make clear to your reader what you intend, 
> and makes it easier to spot something you didn't intend or mean to do.
> 
>     cd /root/ldapca
> 
> This might just be my personal preference, but if I were providing a 
> critique to one of my students or engineers, I would say don't do cd in 
> your scripts unless it is absolutely and completely necessary, which it 
> almost never is.  You're globally altering the context of your script's 
> execution and then depending on a relative path to be correct.  An 
> altered context like this is very, very hard to keep track of and makes 
> it much harder to read into what the full path is later in a script, 
> separated from this cd command.
> 
> 
>     [ $? -eq 0 ] && echo "localhost ok" || { echo "localhost not ok"; 
> exit 1; } ;;
> 
> Be very sparing about using complex expressions like this.  Within 
> reason, if there is a more readable way to write something, choose 
> readable over terse.  That's a general critique both for your future 
> self and future maintainers.  Try not to bury critical code branch 
> changes (ie terminal fault handling) inside a single line like that.  It 
> makes it easy to miss the 'exit 1' when reading and troubleshooting --- 
> and makes it much harder to modify the behavior later if needed.
> 
> I know it looks cooler and more geek-cred to write complex single line 
> chains, but it is much less readable and far less maintainable over time.
> 
> I worked with a coder some years back who wrote terse, complex messes 
> because he thought it made him look smarter and gave him the false sense 
> that he alone owned his code. No one else could understand it enough to 
> monkey with it.  Needless to say, he was lousy at his job because in the 
> real world people have to collaborate.  I wasn't out to change his code 
> - but I was trying to use it.  Without much documentation or commenting, 
> I was forced to try to wade through the swamp of frustration - wasting 
> tons of time.  When something he wrote wasn't working properly, it would 
> take much longer to get it fixed.
> 
>     [ -f /etc/openldap/changes.ldif ] && { ldapmodify -Y EXTERNAL -H 
> ldapi:/// -f /etc/openldap/changes.ldif; } || { echo "failed to modify 
> changes.ldif"; exit 1; }
> 
> That line is too long and as mentioned above, too complex for a single 
> line.  It is too complex because you're squashing tests and 
> object/state/system modifying actions and a terminating fault command 
> into a single line - basically one long statement.  Break it down - 
> while it takes more lines, it is much more readable and easier to maintain.
> 
>     [ -f /usr/share/migrationtools/migrate_common.ph ] && { sed -i 
> 's/"padl.com"/"example.com"/' 
> /usr/share/migrationtools/migrate_common.ph; } && { sed -i 
> 's/"dc=padl,dc=com"/"dc=example,dc=com"/' 
> /usr/share/migrationtools/migrate_common.ph; }
> 
> At the risk of sounding like I'm harping on the same theme, please don't 
> do this.
> 
> Here's a rule of thumb - if a line seems like a run-on sentence, it 
> probably is.
> 
> Sometimes long lines (some of your certutil commands) are reasonably 
> unavoidable.
>> From an output perspective, if you have more than one of a particular
>> network interface (eth0, eth1, for example), your output doesn't readily
>> identify which one works or doesn't work, which makes troubleshooting a
>> bit more difficult.  Again, with the types of systems you are probably
>> running this on, it probably doesn't significantly affect you, but it is
>> something to think about as you develop more complex scripts with
>> additional inputs and outputs. You might want to include $netw in your
>> output to identify the interfaces.
> Agreed.  Try also in general to do a little bit more error checking, 
> with reasonable explanations and if possible a resolution or two for a 
> failure.
> 
> You do some sanity checking, but you also run very long series of 
> commands without making sure any of them worked before moving on.  As 
> Stephen points out, it may not matter too much here but it is something 
> to think about.
> 
> Along these same lines, be careful with the append redirect.  It takes 
> more work, but it is useful to check before using an append to make a 
> configuration change.  Say the script got half-way through and broke.  
> Fix, run it again - the appends that happened before the script broke 
> are going to happen again - duplicating configuration inside your config 
> files which is messy and may have unintended consequences.
> 
> Using >> when you're writing to a log file - who cares.  Modifying 
> pre-existing configuration files is a little different case.
>> 
>> I do see one particular error in the host networking case script. Line
>> 26 is attempting to cut information from a string, but the cut command
>> takes a file as input.  To get this to work (you don't actually use it,
>> because you have the lines that use it commented out), you would need to
>> do an "echo $netw | cut".
> 
> At the end of each case statement, put the closing ;; on its own line 
> for readability.
> 
>> 
>> An interesting thing you might want to look into to bring your scripting
>> to the next level.  You have a couple of places where you do several
>> echo statements that append to a file (lines 124-131 and 148-150, for
>> example).  You could make these a little easier to read (and save
>> yourself some keystrokes, which are always prone to typos) if you use
>> "cat" and a "heredoc", like:
>> 
>> cat<<EndofLdapConf>>  /etc/openldap/ldap.conf
>> URI     ldap://instructor.example.com
>> BASE     dc=example,dc=true
>> TLS_REQUIRE allow
>> EndofLdapConf
> +1 to the HEREDOC suggestion.  The way you did it is fine - there's 
> nothing wrong with how it is.  Stephen is correct though, for a long 
> block like this it might be a little better as a HEREDOC statement.
> 
> 
>     #enable logging
> 
>     echo "local4.* /var/log/ldap.log" >> /etc/rsyslog.conf
>     systemctl restart rsyslog
> 
> Move this to be way earlier in your script.  Nothing about what you're 
> doing depends on it being at the end, but it could help a great deal in 
> troubleshooting to have the logs available before starting slapd, ssd, etc.
> 
> 
> One last thing - the last thing, actually - try to explicitly exit with 
> 0.  It is not required, it will happen automagically etc etc - having it 
> as the last thing you do will make your script a bit more readable and 
> assure your reader that a) you intend to end things at this point and b) 
> they can reasonably expect the return value / exit code to be 0 unless 
> something has gone wrong.
> 
> Overall, this looks pretty good.  Keep at it.
> 
> exit 0
> 
> _______________________________________________
> colug-432 mailing list
> colug-432 at colug.net
> http://lists.colug.net/mailman/listinfo/colug-432