Skip to content

Allow configuration of cluster name, locale & encoding #232

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.

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

Merged
merged 4 commits into from
Jun 28, 2018

Conversation

myii
Copy link
Contributor

@myii myii commented Jun 21, 2018

Commit: Allow configuration of cluster name, locale & encoding

  • name replaces literal string main
  • locale and encoding currently default to C and SQL_ASCII respectively -- obvious benefit to being able to configure these
    • Using the long form switches for both of these (i.e. --locale= & --encoding=), which have identical usage in both initdb and pg_createcluster

Commit: Remove trailing slash so pkgrepo.absent works for Apt

  • Trailing slash gets removed by pkgrepo.managed so then not found by pkgrepo.absent

Commit: Fix pillar.get for MacOS user/group early lookup

  • Noticed this while working on this PR
  • Untested but surely the periods need to be replaced by colons, right?
  • If not, will rebase to remove this commit

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

Sample output

Based on pillar data:

postgres:
  version: '10'
  cluster:
    name: testing07
    locale: en_GB.UTF-8
    encoding: UTF8

Result:

      ID: postgresql-cluster-prepared
Function: cmd.run
    Name: pg_createcluster 10 testing07 -d /var/lib/postgresql/10/testing07 --locale=en_GB.UTF-8 --encoding=UTF8
  Result: True
 Comment: Command "pg_createcluster 10 testing07 -d /var/lib/postgresql/10/testing07 --locale=en_GB.UTF-8 --encoding=UTF8" run
 Started: 07:58:59.222295
Duration: 3380.625 ms
 Changes:   
          ----------
          pid:
              22103
          retcode:
              0
          stderr:
          stdout:
              Creating new PostgreSQL cluster 10/testing07 ...
              /usr/lib/postgresql/10/bin/initdb -D /var/lib/postgresql/10/testing07 --auth-local peer --auth-host md5 --encoding UTF8 --locale en_GB.UTF-8
              The files belonging to this database system will be owned by user "postgres".
              This user must also own the server process.

              The database cluster will be initialized with locale "en_GB.UTF-8".
              The default text search configuration will be set to "english".

@aboe76 aboe76 requested a review from noelmcloughlin June 21, 2018 10:46

#Early lookup for system user on MacOS
{% if grains.os == 'MacOS' %}
{% set sysuser = salt['pillar.get']('postgres.user', salt['cmd.run']("stat -f '%Su' /dev/console")) %}
{% set sysgroup = salt['pillar.get']('postgres.group', salt['cmd.run']("stat -f '%Sg' /dev/console")) %}
{% set sysuser = salt['pillar.get']('postgres:user', salt['cmd.run']("stat -f '%Su' /dev/console")) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch.

@@ -76,7 +76,14 @@ postgresql-cluster-prepared:
- name: {{ postgres.prepare_cluster.command }}
- unless: {{ postgres.prepare_cluster.test }}
{%- else %}
- name: {{ postgres.prepare_cluster.pgcommand }} {{ postgres.data_dir }}
{%- set cc_cmd = '{0} {1}'.format(postgres.prepare_cluster.pgcommand, postgres.data_dir) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Locale is unset by default. So if value is set thats means locale pillar was set.

Could you move this Jinja logic into map.jinja instead of init.sls file?

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

@myii LGTM & nice overall. I have just one comment regarding placement of jinja logic before merging.

@myii
Copy link
Contributor Author

myii commented Jun 24, 2018

@noelmcloughlin Thanks for the review. Added another commit to move the logic to map.jinja.

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!!

@myii
Copy link
Contributor Author

myii commented Jun 28, 2018

You're welcome, @noelmcloughlin. Anything else required for the merge?

@aboe76 aboe76 merged commit 332fe7b into saltstack-formulas:master Jun 28, 2018
@aboe76
Copy link
Contributor

aboe76 commented Jun 28, 2018

@myii thanks for this, @noelmcloughlin thanks for reviewing.

@myii myii deleted the PR_cluster branch June 28, 2018 16:21
@myii
Copy link
Contributor Author

myii commented Jun 28, 2018

Thanks @aboe76.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants