METRON-720 modify generate-md.sh to re-throw errors from within 'find'

[mattf-apache]

This patch also fixes, en passant:
METRON-719 use of quadruple back-ticks in README.md file
which is the problem that caused the need for METRON-720.

[JonZeolla]

Nice catch. When I get to my laptop I'll throw you a PR with some of my standard bash script features, since it's getting modified anyway. Just a couple of minor things.

[JonZeolla]

PR to your branch is open. Feel free to merge --squash without attribution, or not merge and go forward without it. +1 (non-binding) the current state by inspection.

[mmiklavc]

Tested branch locally, with and without 4 backticks. Looks good to me. +1

Side note, I noticed that the sensors link doesn't go anywhere.

[mmiklavc]

Hm, that's only when building with the bin scripts. re sensor links

EDIT: nevermind - found the problem was something local. Looks good.

[mattf-apache]

@JonZeolla , sorry traveling yesterday. Will look at it this evening.

[mattf-apache]

@JonZeolla , thanks for the suggestions. Incorporated the SIG traps and better exit codes, as well as improvements to the 'find -exec' error catching logic. Also added 'set -e' and trap on ERR, and fixed an exit code in the fix-md-dialect.py script.

Changes related to $() vs ``, and explicit declaration of array variables that are being initialized on first use, are valid alternative syntax, but my usages are equivalent. I consider the variants I used to be less cluttered, so I kept them.

While I was at it, I also fixed METRON-717, splitting site.xml into a tracked template file and an untracked auto-generated portion, in git.

[JonZeolla]

Sounds good to me. I typically use declare for all variables just to be explicit and obvious with my intent, and use $() as a standard for readability and easy nesting as opposed to ``. That said, as you mentioned, they are definitely equivalent to what you have across the board.

I did a quick review on my phone, but there are enough changes it probably merits a run up and validation on a screen larger than 5". That said, I would consider adding something like:

If [ -e $fullpath/site.xml.bak ]; then
    mv $fullpath/site.xml.bak $fullpath/site.xml
fi

To the quit function to reinstate the prior site.xml (replacing $fullpath appropriately). I'll try to spin this up today if I can because I'm gone all next week without access to the real world (i.e. Internet).

[JonZeolla]

I'm not going to have time to review this more than I already have. +1 (non-binding) via inspection, pending an addition of my last post's if statement to the quit function. If the PR is still open in 1 1/2 weeks I can take another stab at a review, but don't wait on me.

[mattf-apache]

Hi @JonZeolla , thanks for taking a look amidst the other demands on your time.

I disagree with 'mv $fullpath/site.xml.bak $fullpath/site.xml' for the following reasons:

• site.xml is now a fully auto-generated file (like the contents of site/markdown/), and is no longer tracked in git. That was the METRON-717 improvement. Thus, there is no reason to restore it.
• There are many other artifacts of bin/generate-md.sh besides site.xml, that cannot be restored to former state, and that are likely to be just as broken as site.xml if the build is interrupted. I don't want to give the impression that we've restored a valid overall context.
• Leaving the entire state of the auto-generated content after an interrupted build, including site.xml, may give valuable information about the root cause of the break.

Please accept my not incorporating this change :-)

BTW, for Jon and all reviewers, I should mention that after this patch I did a full re-validation of the site-book, both by:

• visual inspection in a browser of every page of the resulting book, and by
• recursive diff of both the generated MD source files and resulting HTML book files, before and after this whole PR patch.

No unexpected differences were found. Thanks.

[cestella]

Ok, yep, I'm +1 on this. Also, I agree with the reasoning around not having a bak file. Especially compelling is the first bullet point that site.xml is generated now.

[JonZeolla]

I surprisingly found time to take another look. I'm a -1 (non-binding) on this for now, primarily because generate-md.sh fails to run on CentOS 6.8 (where I tested it). The error was basename: invalid option -- 's'.

I think I able to fix the issue by changing that line to item_name=$(basename ${md%.*}) (feel free to sub backticks, just hard to show in GitHub md)

EDIT: Ran this up really quickly on my Mac. The above fix seems to work there too. Didn't take a very deep look though.

Regarding the prior discussions
I guess I fail to see the reason why site.xml.bak is created in the first place then. A reasonable alternative in my mind would be to add something like diff $fullpath/site.xml $fullpath/site.xml.bak && rm $fullpath/site.xml.bak to quit() so at least the bak file has some sort of purpose, otherwise I'd remove it entirely.

[cestella]

Either as @JonZeolla says or you could also do basename $md .md. I do think running on centos is important as eventually I'd like to get this running as part of travis, so I'm willing to hold my +1 until the basename issue is corrected.

[ottobackwards]

re: "site.xml is now a fully auto-generated file" that would be why it is showing in git as unstaged but modified?
Is there a way we can stop that from happening? If it is generated does it need to be in git?

[cestella]

@ottobackwards yeah, we probably need site.xml to be in the .gitignore, right?
EDIT: Retracting, it is in .gitignore

[mmiklavc]

Hey @ottobackwards site.xml is now in .gitignore with @mattf-horton's latest changes.

[ottobackwards]

ok - i'm not building this branch, just what i've seen from other builds

[mattf-apache]

Hi @JonZeolla , @ottobackwards , @cestella , @mmiklavc ,
Thanks to all of you for looking at this. Taking issues in order:

1. Absolutely right, no reason to have site.xml.bak any more! Missed that, will fix momentarily.
2. I'll have to look at that basename issue, but will fix and test on Centos. Glad you noticed, and sorry about that.
3. The weird behavior with site.xml and .gitignore is because the "git rm site.xml" command and the change to add "site.xml" to .gitignore, were squashed into a single commit. If you do it as a sequence of two commits then it transpires cleanly. But I was trying to respect the preference for squashing commits. It won't manifest in fresh "git clone"s, but is a nuisance for folks pulling into an existing branch.

Since I have to make another commit to fix #1 and #2, I will also split #3 back into separate commits. Thanks.

[mattf-apache]

I have pushed fixes for all of the above. To fix item 3 I had to change history, so you'll need to "pull -f" to update an existing image.

I tested code fragments and confirmed that "basename -s" works fine on Centos7 but breaks as described by @JonZeolla on Centos6. The revised code fragments work on both and on Mac OS X.

I'm now testing the whole site-book implementation on both Centos 6 and 7. Will report results.

[JonZeolla]

Spun it up on CentOS 6.8 and macOX 10.12.3, and did a quick code review. Spot tested the traps as well. +1 (non-binding).

The only thing I noticed is I don't think this is necessary due to the > in the following line.

[mattf-apache]

I've tested it to completion on both Centos 6 and 7. It works and generates identical src and site file trees. I also tested the trap on SIGINT, which worked on all three platforms. I tested the trap on ERR but only on Mac.

@JonZeolla , the 'rm -f outfile' before 'blah 2> outfile' may be unnecessary. I did so on the premise that the file might pre-exist and be readonly. In that case, I'm not sure whether the '>' operator deletes the file, or attempts to rewrite it; the former would succeed, the latter would fail. Whereas the 'rm -f' is guaranteed to succeed, because we earlier tested the parent directory for write privs.

At any rate, as you observed, it is at worst unnecessary but harmless. Thanks for the +1.

[mattf-apache]

I just checked, and this PR branch rebases to master without conflict, which I think means it will merge without conflict. If anyone would consider committing it, let me know if you need me to rebase it first. Thanks.