W/A hardcoding of debian alternatives priority

[noelmcloughlin]

The current hardcoding of 'postgres.alternatives.install.priority=30', breaks formula reruns.

- - priority: 30

This PR is a workaround solution, by exposing the value as a configurable pillar.

+ - priority: {{ postgres.linux.altpriority|int }}

Implementation decisions

1. To mimic existing behaviour the alternatives.priority value of '30' is retained by defaults.yaml.
2. Users can disable Debian Alternatives - new feature - by setting '0' as pillar value (that would be better formula default IMHO but lets go one step at a time).

[noelmcloughlin]

Would anyone be able to review?

[EvaSDK]

I don't think debian alternatives makes sense on os_family other than Debian so I'm not sure the conditional is right.

[noelmcloughlin]

I think debian alternatives is a historical name. Its called 'alternatives' on RedHat for example-

alternatives is a tool for managing different software packages that provide the same functionality. Red Hat Enterprise Linux uses alternatives to ensure that only one Java Development Kit (JDK) is set as the system default at one time.

[noelmcloughlin]

BTW, this PR does not add alternatives feature, just removes a hardcoding of 30 priority.

There is a pre-existing check for os_family which do not support 'debian alternatives'.

{% if grains.os_family not in ('Arch',) %}

[EvaSDK]

Gentoo has eselect which is somewhat similar but has its own module https://docs.saltstack.com/en/latest/ref/states/all/salt.states.eselect.html. Not sure alternatives would work there.

[noelmcloughlin]

Good point @EvaSDK Could you raise issue? BTW, such problems exist as legacy technical debt.

[noelmcloughlin]

Hey @EvaSDK I updated commit for your comments. Was thinking this PR assists future eselect support because pillar is reusable for that feature. Probaby eselect has priorities too?

[vutny]

I like the idea of this PR. Having configurable priority for alternatives is a neat feature.

Let's make an implementation more robust. I have few concerns, let's address them one by one as usual.

[vutny]

You don't need to hard-code the list of distros in the state.
The top-level if statement checks if bin_dir parameter has been defined. And it is explicitly defined only for RedHat in osmap.yaml for now. It does not exist for any other system.

Same applies to the server state.

[noelmcloughlin]

Thats correct. The Jinja is unnecessary here. done.

[vutny]

At first, you don't need to introduce lower level if statement, it's better to collapse two statements into one simply using and operator. It reduces amount of changes, unnecessary indentations and makes it more readable overall.

At second, no need to use int filter here. You could just test with implicit Boolean comparison:

  {%- if 'bin_dir' in postgres and
          postgres.linux.altpriority %}

This would negate the check if postgres.linux.altpriority would be 0, '',false or none. And that looks better.

[noelmcloughlin]

Nice suggestion. This is all done.

[vutny]

As well, the int filter is unnecessary. YAML renderer does not care about Jinja internal data type.

[vutny]

Since for now the formula is able to provision the alternatives only for RedHat systems when PG comes from upstream repo, I think the default value across all supported distros must be empty:

linux:
  # Alternatives are applicable only for some OS
  altpriority:

Please set actual value in osmap.yaml file near bin_dir and *_bins parameters.

[vutny]

Awesome. I tested this on CentOS and Ubuntu and it works like a charm.

Thanks for you contribution @noelmcloughlin