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

[Rick Hornsby]

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