Skip to content

Update dependencies#48

Merged
free merged 3 commits intofree:masterfrom
burningalchemist:feature/update_deps
Nov 26, 2019
Merged

Update dependencies#48
free merged 3 commits intofree:masterfrom
burningalchemist:feature/update_deps

Conversation

@burningalchemist
Copy link
Copy Markdown
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
Copy Markdown
Owner

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
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Owner

@free free left a comment

Choose a reason for hiding this comment

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

Fair enough, then.

LGTM

@burningalchemist
Copy link
Copy Markdown
Contributor Author

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

@free
Copy link
Copy Markdown
Owner

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
Copy Markdown
Contributor Author

@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
Copy Markdown
Owner

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
Copy Markdown
Contributor Author

@free No worries, thanks for this! 👍

@burningalchemist
Copy link
Copy Markdown
Contributor Author

@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 burningalchemist deleted the feature/update_deps branch December 1, 2019 16:06
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