> I didn't take time to run the tests or try the installation, but can do so
> tomorrow if desired.  I assume if there were test issues, they'll show up in
> the autopkgtest runs.

I can test that Bryce, thanks a lot for reviewing this for me!
 
> 
> * Code review on patches:
>   - ubuntu/lp1895728-669f13b5-Fix-bootstrap-Change-condition-to-add-stonith-
> sbd-re.patch
>     + Code changes looks correct & safe
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + "bsc#1166967" sounds like a bug tracker ticket but I could not find a
> public bug system with that identifier.  If there is one, it would be useful
> to link to it via a "Bug: ..." DEP3 tag.  From the upstream commit entry
> there's a link to the pull request which has some discussion, so lacking
> access to the 'bsc', you could add "Bug:
> https://github.com/ClusterLabs/crmsh/pull/545".

YES! I forgot about bsc# numbers and the original bug reference! These are: https://bugzilla.suse.com/show_bug.cgi?id=XXXXX as crmsh is maintained mostly by SUSE. Unfortunately they don't keep opened bug reporting systems like we do, so some of those references are not worth adding as there is noting to read. Ill try to replace dos with github issues if appropriate.

I have also forgotten to update the Origin with the commit SHAs.  Doing that..

> 
>   - ubuntu/lp1895728-997f62d6-Fix-hb_report-disable-dump-all-tasks-stack-into-
> dmes.patch
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + Add Bug: https://github.com/ClusterLabs/crmsh/pull/486 (or bsc#1158060)
>     + It looks like the important piece of this patch is dropping the "echo t
> > /proc/sysrq-trigger" line.  However it is also adding dump_D_process() to
> add some process state info; dump_D_process() is running a few bash commands
> which feels a bit janky.  Since this is not an SRU I think it's good enough,
> but if it was an SRU I'd suggest dropping dump_D_process() and carrying only
> the actual fix.

Yep, I don't find nice as well .. things like get_stdout("cat /proc/XXX").. I see your point and agree. For stabilization is good enough, as you said.

>   - ubuntu/lp1895728-cce28254-Fix-crmsh.spec-using-mktemp-to-create-tmp-file-
> bsc-1.patch
>     + LGTM.  Personally I'd use $() rather than `` but this is technically
> correct and using mktemp is a good improvement.
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + Add Bug: https://github.com/ClusterLabs/crmsh/pull/502 (or bsc#1154163)

Alright.

>   - ubuntu/lp1895728-e098387e-Fix-bootstrap-use-csync2-f-option-correctly-
> bsc-1166.patch
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + Add Bug: https://github.com/ClusterLabs/crmsh/pull/542 (or bsc#1166684)
>     + Code changes look good per the pull request, although I wonder if
> csync2_update() should throw an exception if the second `csync2 -rxv` fails,
> but the warning is at least an improvement over the prior code.
>     + Just a nitpick but I notice this patch uses "synced" in one message and
> "sync'd" in another.

Yep :\. I tend not to change upstream unless we face the bug ourselves and suggest the changes. To be honest, bootstrap is something from crmsh we don't usually use much (although we could start using it). There is algo the fact that crmsh "might" be changed - from being the preferred way of configuring our clusters - to PCS (its "competitor"). So, its tricky if we should dedicate much time to crmsh or not. I, personally, like it better... but PCS seems to be gaining big attention and becoming a reference.. lets see...

>   - ubuntu/lp1895728-ca1f0a7f-Fix-ui_resource-refresh-Tab-should-complete-
> resource.patch
>     + Code changes looks correct & safe
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + Add Bug: https://github.com/ClusterLabs/crmsh/pull/561 (or bsc#1167220)

Done.

>   - ubuntu/lp1895728-78ce77e4-Low-hb_report-Fix-collecting-of-binary-data-
> bsc-1166.patch
>     + Fix Origin: url is incomplete (needs SHA appended)
>     + Add Bug: https://github.com/ClusterLabs/crmsh/pull/610 (or where ever
> bsc#1166962 is)
>     + There are a couple potential issues I spot with this patch (see inline
> comments), I think it'd be worth your doublecheck.

Okay.

> The Origin and Bug issues are the main things needing fixed, the others I flag
> mainly just FYI - would be nice to see fixed but not required or expected.

Okay, let me go through your comments over the code now, the commit IDs and bug references are already fixed and will push it when my answers to your review are done.

Thanks a lot for reviewing this!