Some additional updates to Readme.md

[Hooverdan96]

Found a couple of cosmetic adjustments.

Also, I am not clear on this statement:
...
The prior installs rockstor-pre.service (initrock) would have asserted these conditions.
...

[Hooverdan96]

@phillxnet, I made some more adjustments that I noticed could make sense. But still have the above question on the statement that wasn't clear to me. If you have a suggestion for clarification I can also pop that into the PR.

[phillxnet]

@Hooverdan96 Re my:

The prior installs rockstor-pre.service (initrock) would have asserted these conditions.

in it's context of:

The rpmbuild process, within the %check scriptlet, also runs all our existing unit tests. As such a properly configured and running Postgres server is required. The prior installs rockstor-pre.service (initrock) would have asserted these conditions.

So my intention was to inform the user that the 'required for test to run' setup of the postgresql server was already done by the prior install. More specifically the rockstor-pre systemd service which in turn runs our initrock script. This is important as otherwise the %check scriptlet run by rpmbuild would fail. The postgresql setup is non trivial and involves both a tuning phase and an host base authentication phase. We need not repeat that all here as it is already defined by itself in the initrock script. Any duplication of what is done in initrock will inevitably become out-of-date in our explanation here. Hence dropping in that it is required. Folks can look to initrock to see what is done but our purpose here was to provide folks with the requirements to do test builds. There in-turn depend on the tests running as otherwise our production builds fail.

Hope that explains stuff as the Postgresql prep is a bit involved and does include a tuning phase so changes to that tuning could itself fail tests if done incorrectly. Thus the reference to what is required (initrock script). It's why we prompt folks at the end of our build.sh with the following:

https://github.com/rockstor/rockstor-core/blob/8f9705f06ccc267b5252b6d46831adc3b4bfab77/build.sh#L63-L70

Incidentally there were quite a few improvements and clarifications in this db setup in initrock so it's worth taking a look to see what is done. We also log what we do fairly extensively as we go give it helps in diagnosing future issues. And our recent addition of the tuning phase is also most welcome as we can then hopefully attract domain experts in that area to see, in abstraction, what we do. And for folks to change their own setup of course.

We have to be careful not to try and explain all things in all places. This repo is for rpm building only. Not how we setup our database. Hence referencing the system service required to do this, and in turn the actual script that service runs. Ergo the notes printed at the end of the build.sh as that will enable setup for folks to run tests for example also.

I'm personally happy with the depth of explanation on this front as it would entail something like the above which is already almost the size of our entire readme here again. So it's basically domain specific knowledge outside of the build process but pointed to exactly via the script reference we have. That could be enhanced but the systemd service has already run, it may only be that postgresql service is not running that could fail the build but not if the instructions are followed; and we do reference that this service is required to be running:

As such a properly configured and running Postgres server is required.

The actually details of this are a moving target anyway from the point of view of this rpm-build config: but are in turn documented both in code, in logs, and in pull requests and linked issues as we go. So too much specificity means all things everywhere all at once as it were. Not ideal for focused documentation required to all folks to prove their changes will not break our/their existing rpm-build changes.

Haven't looked at your related pr just yet so will go there next.

Thanks again for your 'eyes' on all these things by the way. Much appreciated. In part I think what we need is a boot-up doc section. Explaining at a high to medium level, what each system service is responsible for, and its entry point via our scripts etc. That would be a nice doc entry for on-boarding developers I think. And we could in turn link to that here in this readme at the location you indicate that I did 'wavy hands' on that database requirement.

[Hooverdan96]

Thanks for the detailed explanation, as always. And certainly do agree with the approach to not repeat everything everywhere, especially if you have (and are planning more) cross-references for the details.

So I think, then I just got hung up on the syntax of that sentence (the non-native speaker of me). I did interpret it as:

This setup (postgresql) would have been asserted by the previous install's rockstor-pre.service - without knowing the details that you then explained to me about. But I just wanted to make sure.

[phillxnet]

Closing as:
Fixed by #5