Skip to content
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

[postgresql] Collect data for postgreSQL from RHSCL #1118

Closed

Conversation

pmoravec
Copy link
Contributor

@pmoravec pmoravec commented Oct 3, 2017

Collect postgreSQL data also when postgreSQL is installed from
Red Hat Software Collections.

Resolves: #1090

Signed-off-by: Pavel Moravec pmoravec@redhat.com


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname email@example.com?

@pmoravec
Copy link
Contributor Author

pmoravec commented Oct 3, 2017

There was an idea to have a child class of RedHatPostgreSQL that would bring even simpler patch. But that would be against the principle that sosreport executes exactly one class from each plug-in (cf. with https://github.com/sosreport/sos/blob/master/sos/policies/__init__.py#L239, breaking this would allow two plugins of "postgresql" name, quite confusing).

def setup(self):
if self.is_installed('postgresql'):
self.pghome = self.get_option("pghome")
else: # SCL postgreSQL is installed
Copy link
Contributor

Choose a reason for hiding this comment

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

The patch looks good to me. Just one question, isn't a good idea to check if rh-postgresql95-postgresql-server is installed as you did in line 119?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class can be enabled either by:

The difference is imho small, leaving on @bmr-cymru opinion if it makes sense to improve it that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pmoravec right, what happens if we have both installed? Looks like, it will use the postgresql, which I believe is not correct. A possible solution is invert the if logic, like if self.is_installed('rh-postgresql95-postgresql-server') else postgresql. @sandrobonazzola

cat /etc/redhat-release

CentOS Linux release 7.4.1708 (Core)

[root@localhost ~]# rpm -qa | grep -i postgres
postgresql-jdbc-9.2.1002-5.el7.noarch
rh-postgresql95-postgresql-libs-9.5.7-2.el7.x86_64
postgresql-server-9.2.23-1.el7_4.x86_64
rh-postgresql95-scldevel-2.2-2.el7.x86_64
postgresql-9.2.23-1.el7_4.x86_64
rh-postgresql95-postgresql-server-9.5.7-2.el7.x86_64
rh-postgresql95-runtime-2.2-2.el7.x86_64
postgresql-libs-9.2.23-1.el7_4.x86_64
collectd-postgresql-5.7.2-1.el7.x86_64
rh-postgresql95-postgresql-9.5.7-2.el7.x86_64

Copy link
Member

Choose a reason for hiding this comment

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

Eh, this does not look right to me.

Having separate pghome and sclpghome plugin options is a bit of an ugly hack, and not very nice for users to try to understand.

I think it would be better to revive the SCLPluign mixin concept from PR #900 than to attempt to solve this in an ad-hoc and plugin specific fashion.

Copy link
Member

@bmr-cymru bmr-cymru left a comment

Choose a reason for hiding this comment

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

Let's try to make this work with SCLPlugin if we can - I don't like the proliferation of hacks that are starting to accumulate here.

def setup(self):
if self.is_installed('postgresql'):
self.pghome = self.get_option("pghome")
else: # SCL postgreSQL is installed
Copy link
Member

Choose a reason for hiding this comment

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

Eh, this does not look right to me.

Having separate pghome and sclpghome plugin options is a bit of an ugly hack, and not very nice for users to try to understand.

I think it would be better to revive the SCLPluign mixin concept from PR #900 than to attempt to solve this in an ad-hoc and plugin specific fashion.

@pmoravec pmoravec force-pushed the sos-pmoravec-postgresql-from-scl branch 2 times, most recently from fc15429 to a70b6ff Compare October 16, 2017 11:39
Related to sosreport#900 and sosreport#1090

Original author: Bohuslav Kabrda <bkabrda@redhat.com>
Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
Collect postgreSQL data also when postgreSQL is installed from
Red Hat Software Collections.

Resolves: sosreport#1090

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec force-pushed the sos-pmoravec-postgresql-from-scl branch from a70b6ff to 06adcb2 Compare October 16, 2017 12:13
@sandrobonazzola
Copy link
Contributor

So did this miss sos 3.5?

@pmoravec
Copy link
Contributor Author

It is included in 3.5 via commits 947e708 and 62d6435.

So I am closing this PR.

@pmoravec pmoravec closed this Nov 18, 2017
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.

None yet

4 participants