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

[Tom Hanlon]

I also benefit from discussions like this..

Go COLUG !!!!

Thanks

On Fri, Jun 5, 2015 at 12:01 AM, Zach Villers <zachvatwork at gmail.com> wrote:
> 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
>
> _______________________________________________
> colug-432 mailing list
> colug-432 at colug.net
> http://lists.colug.net/mailman/listinfo/colug-432