Skip to content

Improve the osmapping defaults in osmap.yaml #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

Improve the osmapping defaults in osmap.yaml #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97b82cd
Resolve issue #156; better osmapping
Sep 13, 2017
0fe31a6
Resolve issue #156; better osmapping
Sep 13, 2017
12dab82
Adopt PR comments
Sep 14, 2017
3ba9c85
Adopt PR comments
Sep 14, 2017
cb16d8c
Adopt PR comments
Sep 14, 2017
78f6f74
Adopt PR comments
Sep 14, 2017
e79bb6b
Adopt PR comments
Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions pillar.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,9 @@ postgres:
# Version to install from upstream repository
version: '9.3'

# These are Debian/Ubuntu specific package names
pkg: 'postgresql-9.3'
pkg_client: 'postgresql-client-9.3'

# Additional packages to install with PostgreSQL server,
# this should be in a list format
pkgs_extra:
- postgresql-contrib
- postgresql-plpython
# Package names are defined in codenamemap.yaml and osmap.yaml
# Extra packages: ditto
pkgs_extra: []

# Append the lines under this item to your postgresql.conf file.
# Pay attention to indent exactly with 4 spaces for all lines.
Expand Down Expand Up @@ -58,7 +52,7 @@ postgres:
# Use ``bake_image`` setting to control how PostgreSQL will be started: if set
# to ``True`` the raw ``pg_ctl`` will be utilized instead of packaged init
# script, job or unit run with Salt ``service`` state.
bake_image: True
# bake_image: True

{%- endif %}

Expand Down
32 changes: 26 additions & 6 deletions postgres/osmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,17 @@ Debian:
pkg_dev: postgresql-server-dev-all
pkg_libpq_dev: libpq-dev

pkg: 'postgresql-{{ repo.version }}'
pkg_client: 'postgresql-client-{{ repo.version }}'
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do it here. We have special file codenamemap.yaml to take care of this. The package names are really depending on use_upstream_repo setting.

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree - The dilemma arises because these are defined in pillar.example for Debian, and osmap.yaml for RedHat and I don't like the inconsistency. I moved the Debian variables to osmap.yaml for consistency; but open to suggestions here. Looking at codenamemap.yaml Debian and Fedora are defined today.

pkgs_extra: [ postgresql-contrib, postgresql-plpython-{{ repo.version }}, libpostgresql-jdbc-java]
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure everybody wants all the optional stuff, additional Python and Java dependencies by default?
This makes the formula run longer (read: it's longer to test).

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - we should support pkgs_extra without forcing it.

But I think "pkgs_extra: []" should be default in pillar.example uncommented. Then it should be easy to toggle this by removing "pkgs_extra: []" pillar. This makes regression test and end-user support seamless.

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added pkgs_extra: [] to pillar.example.

Copy link
Contributor

@vutny vutny Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that the formula by default should provide minimum working installation. And in many use cases a user just don't need to wait and download a half of the distribution just to get a separate DB for WordPress backend or something.
I understand that those packages are system-specific, but still I think it is up to the user to figure out and fill in any additional stuff. I suggest to keep pkgs_extra: [] as default and mention it in pillar.example without exposing values for some particular distro.

python: python-psycopg2
{% set data_dir = '/var/lib/postgresql/data' %}
conf_dir: {{ data_dir }}
prepare_cluster:
command: /usr/lib/postgresql/9.6/bin/initdb --pgdata={{ data_dir }}
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You definitely should not do it here. Especially with hard-coded values like 9.6,

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. The default postgres.server state did not work properly. Fixing in pillar.example is considered wrong (see discussion at #156) so where should this be handled?

Yes, This should read {{ repo.version }} not 9.6.

test: test -f {{ data_dir }}/PG_VERSION
user: postgres

FreeBSD:
user: pgsql

Expand All @@ -39,12 +50,6 @@ RedHat:

pkg: postgresql{{ release }}-server
pkg_client: postgresql{{ release }}
conf_dir: /var/lib/pgsql/{{ repo.version }}/data
service: postgresql-{{ repo.version }}

prepare_cluster:
command: initdb --pgdata='{{ data_dir }}'
test: test -f '{{ data_dir }}/PG_VERSION'
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is here for the purpose: the values should be set like this only when use_upstream_repo has been set to True.

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. Note however that initdb, data_dir and conf_dir path can differ between OS.


# Directory containing PostgreSQL client executables
bin_dir: /usr/pgsql-{{ repo.version }}/bin
Expand Down Expand Up @@ -85,15 +90,30 @@ RedHat:

{% else %}

{% set data_dir = '/var/lib/pgsql/data' %}
pkg: postgresql-server
pkg_client: postgresql

{% endif %}

service: postgresql-{{ repo.version }}
pkgs_extra: [ postgresql-contrib, postgresql-plpython, postgresql-jdbc, postgresql-docs]
conf_dir: {{ data_dir }}
prepare_cluster:
command: initdb --pgdata='{{ data_dir }}'
test: test -f '{{ data_dir }}/PG_VERSION'
user: postgres
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these lines are unnecessary, please see defaults.yaml.

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But initdb and data_dir paths can differ between OS in theory.
How can this be addressed? See: #160

Copy link
Contributor

@vutny vutny Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you just moved the section that should be defined for upstream PostgreSQL distribution.
If these parameters are not defined, the corresponding ones from defaults.yaml would be merged in.


Suse:
pkg: postgresql-server
pkg_client: postgresql
pkg_libpq_dev: postgresql
pkgs_extra: [ postgresql-contrib, postgresql-plpython, postgresql-jdbc, postgresql-docs]

{% set data_dir = '/var/lib/pgsql/data' %}
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're going to set global variable, better to put it on top of the file. It would be redefined in if statement when necessary. But you don't need it at all.

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed.

prepare_cluster:
command: /usr/lib/postgresql/bin/initdb --pgdata={{ data_dir }}
Copy link
Contributor

@vutny vutny Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is the only required change for Suse. Are you sure that initdb is not in $PATH?

Copy link
Contributor Author

@noelmcloughlin noelmcloughlin Sep 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command failed on SuSE so that is path.
The command failed on Ubuntu too - path is /usr/lib/postgresql/9.6/bin/initdb #160

test: test -f {{ data_dir }}/PG_VERSION
user: postgres

# vim: ft=sls
2 changes: 1 addition & 1 deletion postgres/repo.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# This file allows to get PostgreSQL version and upstream repo settings
# early from Pillar to set correct lookup dictionaty items
# early from Pillar to set correct lookup dictionary items

{% import_yaml "postgres/defaults.yaml" as defaults %}

Expand Down