Refactor sbd loading

[nelsonkopliku]

Description

Sbd configuration gets read in the two major processes of the agent:

• Discovery (cluster)
• Facts Gathering (sbd_config gatherer)

This PR reduces some duplication of sbd configuration file loading and in discovery/facts gathering.

Notes:
utils.FindMatches('(?m)^(\w+)=(\S[^#\s]*)', sbdConfigRaw) has been superseded by envparse.Parse(sbdConfigFile).
A subtle difference here is that FindMatches ignores empty values like SBD_OPTS= while envparse decodes to empty string.

This means that the cluster discovery would also have "SBD_OPTS": "" among the other configs.

Would this break something? 😅 I tried and it does not break things up.
Should we make sure to skip empty entries when parsing? I believe we should return sbd config entries even if they're empty, see #145 (comment)

[fabriziosestito]

Would this break something? sweat_smile

@nelsonkopliku I think that before considering changing this, this question should be answered by adding automated tests where needed

[fabriziosestito]

in case we could remove the extra entries so the output is the same to what we had with the regex

[arbulu89]

I don't get this. I think we should return the empty values if they are configured
Imagine that you want to create a check to see if the configured value has an empty string.
Like Check SBD_OPTS has not additional values and you see that it is not changed.
I don't know, this hides information.

Not having those values in the past shouldn't be a reason not to add them now.

[nelsonkopliku]

At the end I returned to my first proposal of not skipping empty values in config and covered in test.

If this is sorted out we need to agree on trento-project/web#996.

[nelsonkopliku]

adding automated tests where needed

I made this change on trento web trento-project/web#996 and let e2e tests tell us if integration does not break 😅

Would that be enough?

[nelsonkopliku]

@fabriziosestito made the change you suggested.
Now if an existing entry in sbd config is an empty string, that entry gets removed from the parsed config.

This also means that if we do something like

facts:
  - name: sbd_opts
    gatherer: sbd_config
    argument: SBD_OPTS # or any other possibly empty entry

We'll get a FactGatheringError saying sbd-config-value-not-found.
We can start like this, it's good enough I believe, anyway I see a little discrepancy with the fact that the config value was actually there, it was just empty.
It depends on what meaning we want to give to the emptiness of a config entry and if that state is an error or something we'd like to know as a fact in our expectations.

[arbulu89]

Folks, sorry for arriving late, but I think that the removal of empty values is a bad idea. We are hiding real configuration, that might be useful in the checks for example.
Senging a new field with an empty value shouldn't be a problem for the web, as most probably we don't use that field.

Besides that, I find the error handling way complicated

[arbulu89]

I don't get this. I think we should return the empty values if they are configured
Imagine that you want to create a check to see if the configured value has an empty string.
Like Check SBD_OPTS has not additional values and you see that it is not changed.
I don't know, this hides information.

Not having those values in the past shouldn't be a reason not to add them now.

[arbulu89]

@nelsonkopliku Green light from my side.
I commented 2 minor things that don't need an extra review.