-
Notifications
You must be signed in to change notification settings - Fork 446
Adding postgres version check as per issue #43 #384
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emdem Thanks for the PR! Just one minor change.
pkg/postgresql/utils.go
Outdated
@@ -404,7 +404,7 @@ func getConfigFilePGParameters(ctx context.Context, connParams ConnParams) (comm | |||
|
|||
func ParseBinaryVersion(v string) (int, int, error) { | |||
// extact version (removing beta*, rc* etc...) | |||
regex, err := regexp.Compile(`.* \(PostgreSQL\) ([0-9\.]+).*$`) | |||
regex, err := regexp.Compile(`.* \(PostgreSQL\) ([0-9\.]+).*`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The final $
from the regexp is probably unuseful given the .*
. Have you removed it also for other reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is not useful because .*
and the other reason is that when I ran the tests/debugged on a machine with self-compiled postgres I noticed some the regex failed to match when the output to stdout of the version was terminated with '\r\n'...
pkg/postgresql/postgresql.go
Outdated
@@ -238,6 +238,14 @@ func (p *Manager) Start() error { | |||
} | |||
|
|||
func (p *Manager) start(args ...string) error { | |||
log.Infow("checking postgres version") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will put this to debug level.
@emdem LGTM. Can you please rebase so I can merge it? |
Done! |
The tests seem to be intermittently failing due to timeouts... |
@emdem tests should work correcly, can you please squash and rebase on current master? |
No description provided.