Suse: Introduce support for upstream repo #169

noelmcloughlin · 2017-09-25T00:35:41Z

This PR is an enhancement to allow the upstream repo to be used for SLES and OpenSUSE.

The osmap.yaml was updated introducing the feature. Design is aligned with Redhat Stanza for convenience and continuity. These log files demonstrate working "feature" on OpenSuse LEAP 42 as postgresql 9.5 software is retrieved from upstream-

Limitation
GPGCHECK is disabled (for now): See https://redmine.postgresql.org/issues/2732

Followup

Fix gpgcheck on Suse - Raise ISSUE.

Nice to Have
The pillar.example was improved in the following manner-

Required parameters are prominent. For example the value of service differs depending on whether linux distribution or upstream repo is used.
Unnecessary parameters (pkg, pkg_client, pkgs_extra) were removed to fix confusion.

Thanks

vutny

I welcome a good intension to add upstream support for SUSE, but for me the implementation looks like half-done.
Very quick and rough investigation reveals obvious things. Please take a look at my in-line comments.

Also, would you mind to organize Git history in PR(s) a little bit? You can use --amend to simply add stuff to your previous commit. And do --interactive rebase to squash intermediate commits. This will significantly improve understanding what's changed and why, especially when the PR would be eventually merged.

vutny · 2017-09-26T06:59:36Z

postgres/osmap.yaml

+    name: pgdg-sles-{{ release }}
+    humanname: PostgreSQL {{ repo.version }} $releasever - $basearch
+    #Suse repos are suffixed 'suse/sles-12-$basearch' by upstream community convention.
+    baseurl: 'https://download.postgresql.org/pub/repos/yum/{{ repo.version }}/suse/sles-12-$basearch'


Hard-coding SUSE release number looks not very flexible. Things may change, so maybe use $releasever? However, this may affect OpenSuse...

I cannot use '$releasever' because packages target Suse family (SUSE 12+, OpenSUSE 42+), so the upstream repo convention seems to be hardcoding of "sles-12". Using '$releasever' breaks the formula for Suse family except Sles 12.

Yeah, that's true, I understand. My point was that the installation could be broken on other SUSE variants which is not currently supported by upstream: SLES 11, OpenSuse 13 or Tumbleweed.
In such cases it's better to fail fast. But that also introduces complexities.
I think this is fine for now.

vutny · 2017-09-26T07:38:00Z

postgres/osmap.yaml

+    baseurl: 'https://download.postgresql.org/pub/repos/yum/{{ repo.version }}/suse/sles-12-$basearch'
+    gpgkey:  'https://download.postgresql.org/pub/repos/yum/{{ repo.version }}/suse/sles-12-$basearch/repodata/repomd.xml'
+    #future: fix suse "4-Signatures public key is not available" issue and enable gpgcheck.
+    gpgcheck: 0


I'm not a SUSE expert by any mean, but I assume GPG key verification should just work as in other RedHat-based distro. Have you tried to resolve this? I would make a guess that the key public key file is just located elsewhere.

It looks not so nice to introduce "half-secure" solution by default.

I agree its not nice - I'm looking for agreement to raise as "ISSUE" and fix afterwards.

Yes I done some troubleshooting. But @myoung34 has identified the problem.

You just need to provide correct public key URL, and I think everything would just "magically" work :)

Duh, need to wear my reading glasses.

vutny · 2017-09-26T08:18:56Z

postgres/osmap.yaml

+  {% set lib_dir = '/var/lib/pgsql/' ~ repo.version ~ '/data' %}
+
+  pkg: postgresql{{ release }}-server
+  pkg_client: postgresql{{ release }}-server


As I see, client binaries are available from postgresql{{ release }} package.

vutny · 2017-09-26T09:05:04Z

postgres/osmap.yaml

+    test: test -f '{{ lib_dir }}/PG_VERSION'
+
+  # Directory containing PostgreSQL client executables
+  bin_dir: /usr/pgsql-{{ repo.version }}/bin


Why to blindly copy-paste all the stuff? It seems that SUSE packages install all (?) needed alternatives itself during post-installation phase.

The PR description explains why ... "Design is aligned with Redhat Stanza for convenience and continuity". It keeps the PR simple.

It seems that SUSE packages install all (?) needed alternatives itself during post-installation phase.

The upstream repo is common to SusE/RedHat/Fedora so same comment applies. Perhaps alternatives handling can be improved but lets keep PR focussed.

The SUSE section does need to be full copy of RedHat just for the sake of artificial consistency.
These options bin_dir, client_bins and server_bins serve a concrete purpose: allow subsequent states and postges modules to properly discover binaries and be able to do the job.

Other distros don't have them, because they don't need to. That's why it is configurable.
The necessary thing should be already done by installed packages, so the formula must not duplicate this.

This alternatives cod is legacy. Perhaps in the past the packages did not already do this.

I will investigate and raise common issue on RedHat/Suse.

And Suse definitely doesn't require that.

There has to be a better way to handle this than to cargo cult large blocks that are similar between os families

Thanks for pointing me in right way here. Surely RedHat has same problem.

Agreed - I tested and all except two are already handled by upstream packages.

vutny · 2017-09-26T09:08:54Z

postgres/osmap.yaml

  pkg: postgresql-server
-  pkg_client: postgresql
-  pkg_libpq_dev: postgresql
+  pkg_client: postgresql-server


Again, are you sure the client and the server package are the same in packages provided by SUSE distribution?

Yes, I'm sure.

Actually, it was a rhetoric question :)
https://software.opensuse.org/package/postgresql

I'm sorry, okay thanks.

vutny · 2017-09-26T09:20:06Z

pillar.example

-  pkgs_extra:
-    - postgresql-contrib
-    - postgresql-plpython
+  pkgs_extra: []


I'm aware that hard-coding platform-specific packages here was bad idea. But this possibly may affect later installation of the uuid_ossp extension defined below. Have you tried to run test kitchen with provided config?

No, I've tested on OpenSUSE LEAP 42.

Better to test your changes on other platforms as well, because you change shared things, and pillar.example has been used in existent test. That's not perfect, and the change is actually meaningful, but we need to understand an impact.

I've already been testing formula on other distributions - see #171 #170 #164.

Only regression is #172 which is something you mentioned regarding pkgs_extra()

I assume this affects Ubuntu and the existing test based on it.

Also this is just a pillar.example, just leave the values as is to help people understand layout. We only get one pillar.example and support (unofficially) any number of distros so it doesnt need to change with every iteration of what distro was last used against it.

That's exactly what I've been trying to express 👍 But having some working example that fits the most popular ones would be great.

Agreed. Will action.

myoung34 · 2017-09-26T10:53:45Z

postgres/osmap.yaml

+    humanname: PostgreSQL {{ repo.version }} $releasever - $basearch
+    #Suse repos are suffixed 'suse/sles-12-$basearch' by upstream community convention.
+    baseurl: 'https://download.postgresql.org/pub/repos/yum/{{ repo.version }}/suse/sles-12-$basearch'
+    gpgkey:  'https://download.postgresql.org/pub/repos/yum/{{ repo.version }}/suse/sles-12-$basearch/repodata/repomd.xml'


this isnt a gpg key, this is a yum repo metadata xml file

Good spot, thanks.

I have found that starting from PostgreSQL 9.6 they ship valid repo key within the repo:
https://download.postgresql.org/pub/repos/zypp/9.6/suse/sles-12-x86_64/repodata/repomd.xml.key

I think we could leave 9.5 and versions below as is, since user is able to override GPG check in Pillar.

myoung34 · 2017-09-26T12:00:18Z

I see that there's a .kitchen.yml file. can we get @aboe76 to enable it to prevent some regressions?

vutny · 2017-12-05T14:45:44Z

Hi @noelmcloughlin

Could you please update the status of this PR? Would you have some time or any plans to work on it further according to review comments?
Thanks for your contributions anyway!

noelmcloughlin · 2017-12-05T15:58:56Z

Hey @vutny
To recap, the original PR proposes adding support for OpenSUSE without Hash-checking. PR reviewers preferred full feature rather than incremental improvement. However, full feature is blocked by issues mentioned in #173. The correct action here is to close the PR and I should refactor a new solution acceptable to community.

Thank you for reviewing!!!!! I will followup on #173 and your comments in this review in a new PR as soon as I make some time.

vutny · 2017-12-06T07:59:12Z

@noelmcloughlin Awesome, thanks. So let's keep this PR open as a reminded until you'd have another one refactored. Then we close it out in favor of that.

noelmcloughlin · 2018-01-09T16:54:33Z

Hi @vutny I'll pick this up again as next priority, been hectic with other stuff. thx

noelmcloughlin · 2018-01-14T22:05:32Z

Rebased as #176

Noel McLoughlin added 7 commits September 24, 2017 21:13

Add upstream repo for Suse

5ad8662

Add upstream repo for Suse

a2efcc8

Add upstream repo for Suse

ea82fb1

Add upstream repo for Suse

2a450ac

Add upstream repo for Suse

e73e1fe

Improve pillar.example defaults

09355f7

Improve pillar.example defaults (tested)

16b293d

noelmcloughlin mentioned this pull request Sep 25, 2017

Suse: Fix Suse stanza in osmap.yaml #168

Closed

noelmcloughlin and others added 2 commits September 25, 2017 01:42

Merge branch 'master' into suse-upstream-repo

bca9ec7

restore endif

4e937c6

vutny suggested changes Sep 26, 2017

View reviewed changes

myoung34 reviewed Sep 26, 2017

View reviewed changes

revert pillar.example

601ba2e

noelmcloughlin mentioned this pull request Jan 14, 2018

Support for upstream postgresql.org zypp repo #176

Merged

noelmcloughlin closed this Jan 14, 2018

noelmcloughlin deleted the suse-upstream-repo branch January 14, 2018 22:09

Suse: Introduce support for upstream repo #169

Suse: Introduce support for upstream repo #169

Uh oh!

Conversation

noelmcloughlin commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vutny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin Sep 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelmcloughlin Sep 26, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myoung34 commented Sep 26, 2017

Uh oh!

vutny commented Dec 5, 2017

Uh oh!

noelmcloughlin commented Sep 25, 2017 •

edited

Loading

noelmcloughlin Sep 26, 2017 •

edited

Loading

noelmcloughlin Sep 26, 2017 •

edited

Loading