d4gh0s7 / postgres_dump Goto Github PK

The readme now states:

  vars_files:
    - ./group_vars/all.yml

But, group_vars/all.yml will always be used, no need to include it.

If you want to tell users that there are variables, you can use a couple of formats that are easy to read:

In a vars structure:

  vars:
    variable1: value1
    variable2: value2

Or on inclusion of the role:

  roles:
    - role: myroles
      variable1: value1
      variable2: values2

As a suggestion; prepend all variable with the name of your role: postgres_dump. This helps to:

• understand where the variable will be used.
• duplication of variables used in other roles.

So for example the variable postgres_base_dump_folder would become something like: postgres_dump_base_folder.

You've got some set_fact tasks like:

- name: Determine the postgresql dump file name
  set_fact: postgresql_dump_filename=full_dump-{{ ansible_date_time.iso8601_basic_short }}.sql

That can be moved to vars/main.yml, like this:

postgresql_dump_filename: "full_dump-{{ ansible_date_time.iso8601_basic_short }}.sql"

This keeps the tasks as clean as possible.

And if you need to use set_facts, you can combine all facts into a single set_facts tasks, like so:

- name: do something
  set_fact:
    variable1: value1
    variable2: value2

(See how I did not use the = there? ;-) )

I think there are some places where you can use modules instead of command or shell like:

    - name: Set up the temporary user
      command: psql --command="ALTER USER {{ random_username.stdout }} WITH PASSWORD '{{ random_string.stdout }}';"
      no_log: Yes

Have a look at this list of postgres modules

By the way, I really like the creation and deletion of a temporary user.

You can add "check_mode: yes" to tasks that are shell/command/raw, instructing Ansible to run the task anyway, even though it's running in check_mode. (-C).

This allows consumers to run your role in check_mode.

Also see the docs

The comments are not required, making (meta information of) the role less readale.

For example:

  # - name: SomePlatform
  #   versions:
  #   - all
  #   - 1.0
  #   - 7
  #   - 99.99

The vars/main.yml are used to set variable not exposed to the user, defaults/main.yml is intended to be overwritten (when required) by the user of your role.

These variable look to me like they should be changeable by the user:

# Set to No to skip the service postgresql status task
# When set to No, this variable can be overruled if provision_temporary_user is set to Yes
ensure_postgresql_is_running: Yes

# Set cleanup_after_backup to No, to store the backup files along with the compressed version
cleanup_after_backup: Yes

I don't see the pg_status variable reused any where or mentioned in the documentation.

If there is a strong case for pg_status, document it and probably rename it to postgres_dump_status, see #14