Improved mina.service ExecStart proposal

[lk86]

Continuing the discussion from here: ec68b96

Before/After/Proposed:
The ExecStart directive in 1.1.X Releases:

ExecStart=/usr/local/bin/mina daemon \
  -peer-list-url "${PEERS_LIST_URL}" \
  -block-producer-key "%h/keys/${KEY_NAME}" \
  -log-level "${LOG_LEVEL}" \
  $EXTRA_FLAGS

ExecStart in 1.2.0beta7:

ExecStart=/usr/local/bin/mina daemon \
  -peer-list-url "${PEERS_LIST_URL}" \
  -block-producer-key "%h/keys/${KEY_NAME}" \
  -log-level "${LOG_LEVEL}" \
  $EXTRA_FLAGS

Proposed new ExecStart:

ExecStart=/usr/local/bin/mina daemon \
  -log-level "${LOG_LEVEL}" \
  -file-log-level "${FILE_LOG_LEVEL}" \
  -peer-list-url "${PEERS_LIST_URL}" \
  ${BLOCK_PRODUCER_FLAGS} \
  $EXTRA_FLAGS

I propose the block producer key flag is instead replaced by a variable BLOCK_PRODUCER_FLAGS that defaults to "". This matches the daemon default of not producing blocks and not requiring a key, but also allows easy configuration of this variable to any flag or set of flags to provide their block producer key to the daemon. This also allows importing the key and specifying the block producer key as --block-producer-pubkey or any new flags introduced to the daemon in the future. The docs can easily be generalized to describe how to use this effectively.

Additionally I propose a FILE_LOG_LEVEL flag to match the log level one, defaulting to Info.

In the future it would be useful to also support these flags in exactly the same fashion in the docker start script so that you can pass a .mina-env file into either the debian package or docker without modification.

[jrwashburn]

I think this sums up as, let's put in defaults for log levels and peer list, and let everything else be passed in from the environment. If that's what you intend, I'm all for it.

I propose the block producer key flag is instead replaced by a variable BLOCK_PRODUCER_FLAGS that defaults to "".

Doesn't $EXTRA_FLAGS already accomplish what BLOCK_PRODUCER_FLAGS would? I don't see the benefit of an extra var or another way to pass in more params.

I think I saw earlier discussion/agreement that all params should move to -- (e.g. --peer-list-url vs. -peer-list-url.) If that is the case, the unit file should also be updated to observe the convention.

Note the redundant -peer-list-url in the proposed - I assume that was not intended.

I don't think the quotes or brace expansion is necessary and think plain old $VAR is safer. If it is necessary I'm wondering what the use case is?

[lk86]

Yes the duplicate is a mistake, and great point about --. The braces should not cause any special expansions in this case, and the quotes are useful when the variables contain spaces which could happen for a peers list url. This is unlikely to matter for log levels but doubtful that it would hurt. The quotes were especially useful for file paths, but this setup removes the need for those (and instead puts a burden on the user to properly quote everything). I see your argument for skipping the block producer flag alltogether and just documenting using extra flags for this purpose, so I have no included that in the PR here: #9533

[ivandiazperez]

For me the extra flags in .mina-env are confusing and do not see any benefits in using them. I would avoid nesting the block producer flags as well, why not just declare them on start explicitly and even enforce those where a certain default is wished? For example, if we consider that de default error log level should be info, we should ask the user to declare it, so everyone has to make a conscious choice. I really prefer to know what is going on once I run the service.