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

Update dependencies #48

Merged
merged 3 commits into from Nov 26, 2019
Merged

Update dependencies #48

merged 3 commits into from Nov 26, 2019

Conversation

@burningalchemist
Copy link
Contributor

@burningalchemist burningalchemist commented Nov 19, 2019

Hi @free ,

Here is one of the pull requests I mentioned earlier.

We want to update vendor dependencies, among which:

  • libpq - contains connection leaks bugfixes, Pinger interface, etc.
  • mysql - similar improvements
  • prometheus/client_golang - became v1.x with stable API and several bugfixes
  • ClickHouse - kshvakov/clickhouse was officially moved to ClickHouse/clickhouse-go

Additionally, I've added appengine into ignore (vendor.json) as we don't work with Google AppEngine API here and cleaned up unused packages with govendor remove +u


Few points:

Once Prometheus go client was released as a major version, they also removed some functions and deprecated API, one of which is LabelPairSorter: prometheus/client_golang/pull/453
To use the function where it's needed, they encourage to implement sort interface in place.
We can do the same as they did in Prometheus Pushgateway.

With a major version, Prometheus client uses xxhash package v2, which is referenced via go modules and can't be fetched with govendor because of different branch handling (issue).
To work this around (as the real fix is to migrate to go modules) I had to clone xxhash v2 manually to GOPATH and then applied govendor update to add it to a vendor directory.

It's not the way I'd like to treat this dependency, but it seems to be a rather quick and working solution as the hashing algorithm package doesn't seem to be updated that frequently that it could become a problem. So, in my opinion, it's acceptable.

The next step, of course, would be the migration to go modules as it's recommended and also packages become less compatible with govendor. Meanwhile, we get updated dependencies with improvements and bugfixes.

What do you think?

@free
Copy link
Owner

@free free commented Nov 19, 2019

LGTM for the most part, but what's up with the new golang.org/x/sys/windows dependency? Is that being pulled in by something else or did you add it explicitly?

@burningalchemist
Copy link
Contributor Author

@burningalchemist burningalchemist commented Nov 19, 2019

@free I've also noticed that dependency and wasn't too happy about it. It comes from Prometheus since v1.0.0, so it seems we have to live with it. It's also controlled by the build annotation here and isn't used unless it's a Windows build.

@free
free approved these changes Nov 20, 2019
Copy link
Owner

@free free left a comment

Fair enough, then.

LGTM

@burningalchemist
Copy link
Contributor Author

@burningalchemist burningalchemist commented Nov 26, 2019

Hi @free, do you have any ETA on when it can be merged? No hurry, just asking. :)

@free
Copy link
Owner

@free free commented Nov 26, 2019

I was thinking that having approved the PR you would be able to do the honors yourself. Is that not the case?

@burningalchemist
Copy link
Contributor Author

@burningalchemist burningalchemist commented Nov 26, 2019

@free Oh no, I don't have write access to the repository, so I can't merge any of PRs, even approved ones.

@free
Copy link
Owner

@free free commented Nov 26, 2019

Damn, didn't know that. Then I'll merge it myself.

@free free merged commit 7ead955 into free:master Nov 26, 2019
@burningalchemist
Copy link
Contributor Author

@burningalchemist burningalchemist commented Nov 26, 2019

@free No worries, thanks for this! 👍

@burningalchemist
Copy link
Contributor Author

@burningalchemist burningalchemist commented Nov 26, 2019

@free When you have some time, would it be generally possible to bump up the version? According to semantic versioning and my understanding, it probably would be something like 0.5.1, since no new features have been introduced, but dependency changes are quite significant.

Those release docker tags which you set up recently do a really good job for Kubernetes clusters. In this case, relying on a specific version helps us moving forward and rollback automatically, rather than having latest since there's no update detection, so it's really hard to update things in this case.

Otherwise, if you want to add some features before the next 0.x release or work out the pull request list, I'm happy to join. :)

burningalchemist added a commit to burningalchemist/sql_exporter that referenced this pull request Nov 27, 2019
* Update vendored packages with `govendor update +e +v +m`

* Add labelPairSorter type to implement sortable labelPairs

* Update Clickhouse dependency
@burningalchemist burningalchemist deleted the burningalchemist:feature/update_deps branch Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.