Skip to content

Conversation

emdem
Copy link
Contributor

@emdem emdem commented Nov 17, 2017

No description provided.

Copy link
Member

@sgotti sgotti left a 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.

@@ -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\.]+).*`)
Copy link
Member

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?

Copy link
Contributor Author

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'...

@@ -238,6 +238,14 @@ func (p *Manager) Start() error {
}

func (p *Manager) start(args ...string) error {
log.Infow("checking postgres version")
Copy link
Member

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.

@sgotti
Copy link
Member

sgotti commented Nov 30, 2017

@emdem LGTM. Can you please rebase so I can merge it?

@emdem
Copy link
Contributor Author

emdem commented Nov 30, 2017

Done!

@emdem
Copy link
Contributor Author

emdem commented Apr 20, 2018

The tests seem to be intermittently failing due to timeouts...

@sgotti
Copy link
Member

sgotti commented Jun 4, 2018

@emdem tests should work correcly, can you please squash and rebase on current master?

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.

2 participants