Support fromrepo in pkg.installed states #185

noelmcloughlin · 2018-02-05T12:01:35Z

This PR fixes a bug around repo handling.

When packagename exists in multiple repo, setting use_upstream_repo: True cannot guarantee upstream package gets installed. This bug was noticed while testing #176 on openSuse with upstream version: 9.6.

Also resolves #133

noelmcloughlin · 2018-02-05T13:52:08Z

Testing done - works nicely.

vutny

I generally like the idea of using the fromrepo with use_upstream_repo setting.

Unfortunately, these changes do not resolve #133, but break support for upstream on all distros. Also, the fromrepo parameter works differently on apt-based systems. Please take a look at my comments below.

vutny · 2018-02-16T12:33:36Z

postgres/server/init.sls

    - refresh: True
    - require:
      - pkgrepo: postgresql-repo
-{%- endif %}
+
+    {%- do includes.append('postgres.upstream') %}


We have already substituted the includes list above, so this line has no effect and the postgres.upstream states would never be included. It is always better to keep definition and logic around includes statements at the top of SLS file. That looks simpler and makes debugging easier. By removing a single if statement you don't make the code more readable or "optimized".

In support of opinions in #125, its good idea to removing some Jinja in SLS/YML files. These sentiments, which I tend to agree with, can only be addressed in baby steps. No issue here.

I moved this includes down few lines in the code - its the first occurance so surely takes effect.

I saw you did it differently in the client.sls. In THAT case it would work, despite not looking pretty though...

Please double check: moving do includes.append... statement here would break calling the single state like salt-call state.apply postgres.server. The postgres.upstream SLS would be never included this way.

The server SLS is more tricky due to includes Jinja. And reviewing this, maybe the following Jinja must move below postgresql.server.pkg.installed so indent(2) is applied?

{%- if includes -%} include: {{ includes|yaml(false)|indent(2) }} {%- endif %}

The usage of salt.apply is new for me. I'll will report back.

vutny · 2018-02-16T12:40:03Z

postgres/client.sls

 # Install PostgreSQL client and libraries

 postgresql-client-libs:
  pkg.installed:
    - pkgs: {{ pkgs }}
-{%- if postgres.use_upstream_repo %}
+  {%- if postgres.pkg_repo.name %}
+    - fromrepo: {{ postgres.pkg_repo.name }}


This kind of the fromrepo usage would work for YUM etc, but not for APT. Please check out the docs.
You need to provide distribution codename for Debian-based distros. Better to do it by exposing parameter dist in codenamemap.yaml file and use pkg_repo.name only as a fallback.

Thanks, this is good feedback. I have only tested on YUM system, so good catch. I appreciate your PR reviews @vutny, they are always sharp and helpful to community!! I'll do more testing and see if this blocks the PR.

noelmcloughlin · 2018-02-23T16:53:28Z

The latest commit works on Ubuntu, Fedora, Centos7, and OpenSUSE, for downstream.
Needs testing on upstream.

OpenSuse 42.3

----------
          ID: postgresql-cluster-prepared
    Function: cmd.run
        Name: initdb --pgdata=  /var/lib/pgsql/data
      Result: True
     Comment: Command "initdb --pgdata=  /var/lib/pgsql/data" run
     Started: 00:16:43.340343
    Duration: 1796.01 ms
     Changes:   
              ----------
              pid:
                  17563
              retcode:
                  0
              stderr:
                  
                  WARNING: enabling "trust" authentication for local connections
                  You can change this by editing pg_hba.conf or using the option -A, or
                  --auth-local and --auth-host, the next time you run initdb.
              stdout:
                  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 "C".
                  The default database encoding has accordingly been set to "SQL_ASCII".
                  The default text search configuration will be set to "english".
                  
                  Data page checksums are disabled.
                  
                  creating directory /var/lib/pgsql/data ... ok
                  creating subdirectories ... ok
                  selecting default max_connections ... 100
                  selecting default shared_buffers ... 128MB
                  selecting dynamic shared memory implementation ... posix
                  creating configuration files ... ok
                  running bootstrap script ... ok
                  performing post-bootstrap initialization ... ok
                  syncing data to disk ... ok
                  
                  Success. You can now start the database server using:
                  
                      pg_ctl -D /var/lib/pgsql/data -l logfile start

Ubuntu

          ID: postgresql-server
    Function: pkg.installed
      Result: True
     Comment: 2 targeted packages were installed/updated.
     Started: 09:45:18.391633
    Duration: 19893.976 ms
     Changes:   
              ----------
              postgresql-9.5:
                  ----------
                  new:
                      9.5.11-0ubuntu0.16.04
                  old:
              postgresql-client:
                  ----------
                  new:
                      1
                  old:
              postgresql-client-9.5:
                  ----------
                  new:
                      9.5.11-0ubuntu0.16.04
                  old:
              postgresql-client-common:
                  ----------
                  new:
                      173ubuntu0.1
                  old:
              postgresql-common:
                  ----------
                  new:
                      173ubuntu0.1
                  old:
              postgresql-contrib:
                  ----------
                  new:
                      9.5+173ubuntu0.1
                  old:
              postgresql-contrib-9.5:
                  ----------
                  new:
                      9.5.11-0ubuntu0.16.04
                  old:
----------
          ID: postgresql-cluster-prepared
    Function: cmd.run
        Name: pg_createcluster 9.5 main -d  /var/lib/postgresql/9.5/main
      Result: True
     Comment: Command "pg_createcluster 9.5 main -d  /var/lib/postgresql/9.5/main" run
     Started: 09:45:38.295506
    Duration: 5597.467 ms
     Changes:   
              ----------
              pid:
                  34997
              retcode:
                  0
              stderr:
              stdout:
                  Creating new cluster 9.5/main ...
                    config /etc/postgresql/9.5/main
                    data   /var/lib/postgresql/9.5/main
                    locale C
                    socket /var/run/postgresql
                    port   5432

Centos

         ID: postgresql-server
    Function: pkg.installed
      Result: True
     Comment: 2 targeted packages were installed/updated.
     Started: 09:45:03.119051
    Duration: 18191.841 ms
     Changes:
              ----------
              postgresql:
                  ----------
                  new:
                      9.2.23-3.el7_4
                  old:
              postgresql-contrib:
                  ----------
                  new:
                      9.2.23-3.el7_4
                  old:
              postgresql-libs:
                  ----------
                  new:
                      9.2.23-3.el7_4
                  old:
              postgresql-server:
                  ----------
                  new:
                      9.2.23-3.el7_4
                  old:
         ID: postgresql-cluster-prepared
    Function: cmd.run
        Name: initdb --pgdata=  /var/lib/pgsql/data
      Result: True
     Comment: Command "initdb --pgdata=  /var/lib/pgsql/data" run
     Started: 09:45:21.316428
    Duration: 2432.731 ms
     Changes:
              ----------
              pid:
                  50646
              retcode:
                  0
              stderr:

                  WARNING: enabling "trust" authentication for local connections
                  You can change this by editing pg_hba.conf or using the option -A, or
                  --auth-local and --auth-host, the next time you run initdb.
              stdout:
                  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 "C".
               ... etc ....
                  Success. You can now start the database server using:

                      postgres -D /var/lib/pgsql/data
                  or
                      pg_ctl -D /var/lib/pgsql/data -l logfile start

vutny · 2018-02-26T11:39:15Z

The use_upstream_repo: False setting will cause trouble here because it should trigger inclusion of postgres.upstream state.

noelmcloughlin · 2018-02-26T13:58:40Z

The only use_upstream_repo: False setting I can find is in pillar.example and PR does not change. So far I have tested the PR with distro supplied Postgres and False is needed there.

For next test, using upstream, I plan to set use_upstream_rep: True pillar. Is this what you are reminding me about?

vutny · 2018-02-27T15:52:49Z

Oh yes! Sure thing, I meant use_upstream_repo: True of course. 😪

noelmcloughlin · 2018-03-26T09:13:40Z

Testing this today.

noelmcloughlin · 2018-03-28T10:10:30Z

Closing to rebase & submit smaller PRs. Comments here will be adopted.

noelmcloughlin · 2018-04-09T14:06:39Z

New commit done, PR later.

noelmcloughlin mentioned this pull request Feb 6, 2018

Missing fromrepo on postgresql-server and postgresql-client-libs #133

Closed

noelmcloughlin requested a review from tobio February 12, 2018 10:37

vutny suggested changes Feb 16, 2018

View reviewed changes

noelmcloughlin changed the title ~~Fix formula bug: add fromrepo to pkg.installed~~ Support fromrepo in pkg.installed states Feb 23, 2018

fromrepo support

7d4f22d

noelmcloughlin closed this Mar 28, 2018

noelmcloughlin mentioned this pull request Apr 23, 2018

Introduce fromrepo support. #209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support fromrepo in pkg.installed states #185

Support fromrepo in pkg.installed states #185

Uh oh!

noelmcloughlin commented Feb 5, 2018 •

edited

Loading

Uh oh!

noelmcloughlin commented Feb 5, 2018

Uh oh!

vutny left a comment

Uh oh!

vutny Feb 16, 2018

Uh oh!

noelmcloughlin Feb 16, 2018

Uh oh!

vutny Feb 16, 2018

Uh oh!

noelmcloughlin Feb 16, 2018 •

edited

Loading

Uh oh!

vutny Feb 16, 2018 •

edited

Loading

Uh oh!

noelmcloughlin Feb 16, 2018

Uh oh!

noelmcloughlin commented Feb 23, 2018 •

edited

Loading

Uh oh!

vutny commented Feb 26, 2018

Uh oh!

noelmcloughlin commented Feb 26, 2018

Uh oh!

vutny commented Feb 27, 2018

Uh oh!

noelmcloughlin commented Mar 26, 2018

Uh oh!

noelmcloughlin commented Mar 28, 2018

Uh oh!

noelmcloughlin commented Apr 9, 2018

Uh oh!

Uh oh!

Support fromrepo in pkg.installed states #185

Support fromrepo in pkg.installed states #185

Uh oh!

Conversation

noelmcloughlin commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noelmcloughlin commented Feb 5, 2018

Uh oh!

vutny left a comment

Choose a reason for hiding this comment

Uh oh!

vutny Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

vutny Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vutny Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin Feb 16, 2018

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vutny commented Feb 26, 2018

Uh oh!

noelmcloughlin commented Feb 26, 2018

Uh oh!

vutny commented Feb 27, 2018

Uh oh!

noelmcloughlin commented Mar 26, 2018

Uh oh!

noelmcloughlin commented Mar 28, 2018

Uh oh!

noelmcloughlin commented Apr 9, 2018

Uh oh!

Uh oh!

noelmcloughlin commented Feb 5, 2018 •

edited

Loading

noelmcloughlin Feb 16, 2018 •

edited

Loading

vutny Feb 16, 2018 •

edited

Loading

noelmcloughlin commented Feb 23, 2018 •

edited

Loading