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.

Sign up for GitHub

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

Jump to bottom

Add minimal MySQL version to Scraper interface #328

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Add minimal MySQL version to Scraper interface #328

merged 2 commits into from
Oct 29, 2018

Conversation

AlekSi
Copy link
Contributor

@AlekSi AlekSi commented Sep 4, 2018

Closes #327.

@AlekSi
Copy link
Contributor Author

AlekSi commented Sep 5, 2018

The test fails due to me using gofmt from Go 1.11. https://golang.org/doc/go1.10#gofmt specifically recommends against testing this on CI:

Note that these kinds of minor updates to gofmt are expected from time to time. In general, we recommend against building systems that check that source code matches the output of a specific version of gofmt. For example, a continuous integration test that fails if any code already checked into a repository is not “properly formatted” is inherently fragile and not recommended.

AlekSi added a commit to AlekSi/mysqld_exporter that referenced this pull request Sep 5, 2018
@AlekSi
Bump Go versions
848b7c5 
Refs prometheus#328.
@AlekSi AlekSi mentioned this pull request Sep 5, 2018
Closed
@SuperQ
Copy link
Member

SuperQ commented Sep 5, 2018

Yes, you'll have to format with Go 1.10 until we upgrade. I know it's not recommended but we do it as a project for consistency enforcement.

@AlekSi AlekSi force-pushed the scraper-mysql-version branch from 779976e to 83be67a Compare October 29, 2018 05:00
@AlekSi
Copy link
Contributor Author

AlekSi commented Oct 29, 2018

Rebased on top of the current master which uses Go 1.11.
#223 introduced integration testing with Docker Compose on Travis CI, and that PR uses it, but it wasn't enabled in Circle CI configuration. May you help me with this?

@SuperQ
Copy link
Member

SuperQ commented Oct 29, 2018

It looks like this needs a rebase.

@AlekSi AlekSi force-pushed the scraper-mysql-version branch from 83be67a to 7242d8b Compare October 29, 2018 10:08
@AlekSi
Copy link
Contributor Author

AlekSi commented Oct 29, 2018

PTAL

@SuperQ
Copy link
Member

SuperQ commented Oct 29, 2018

Ahh, I see what's wrong with the test. The go test is using SQL directly, rather than using a mock SQL like is used in the other tests.

@AlekSi
Copy link
Contributor Author

AlekSi commented Oct 29, 2018

Exactly. See my comment above.

AlekSi added 2 commits October 29, 2018 17:52
@AlekSi
Add Version method to Scraper interface.
4153b9f 
Signed-off-by: Alexey Palazhchenko <alexey.palazhchenko@percona.com>
@AlekSi
Skip scrapers for unsupported MySQL versions.
c127a55 
Signed-off-by: Alexey Palazhchenko <alexey.palazhchenko@percona.com>
@AlekSi AlekSi force-pushed the scraper-mysql-version branch from 7242d8b to c127a55 Compare October 29, 2018 14:52
@AlekSi
Copy link
Contributor Author

AlekSi commented Oct 29, 2018

Rebased again.

SuperQ
SuperQ approved these changes Oct 29, 2018
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM, this is awesome.

@SuperQ SuperQ merged commit 1465a0b into prometheus:master Oct 29, 2018
@AlekSi AlekSi deleted the scraper-mysql-version branch October 29, 2018 15:39
@AlekSi
Copy link
Contributor Author

AlekSi commented Oct 29, 2018

Thank you. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperQ SuperQ SuperQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@AlekSi @SuperQ