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

Upgrade dependencies. #1170

Merged

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Feb 17, 2017

Brings github.com/gambol99/go-marathon version 0.7.0 0.7.1 (updated, see my comment below).

I previously merged a PR on go-marathon which exposes the task state, which can now be used in Marathon templates. See the upstream PR description for my motivation why I'd like to have the state in Traefik.

Other (auto-)changes to the glide.lock file include upgrading to a later 1.5 client-go commit and removal of an apparently unused dependency (go-metrics).

@timoreimann timoreimann changed the title Upgrade dependencies. [DO NOT MERGE] Upgrade dependencies. Feb 17, 2017
@timoreimann
Copy link
Contributor Author

I just noticed that there's an issue in go-marathon when build with Go 1.8.

Will fix it first upstream and then update this PR.

@timoreimann timoreimann changed the title [DO NOT MERGE] Upgrade dependencies. Upgrade dependencies. Feb 20, 2017
@timoreimann
Copy link
Contributor Author

timoreimann commented Feb 20, 2017

This is ready for review again. The reason for the delay was that Go 1.8 brought in a change to the net/url package which could cause go-marathon to fail handling certain kinds of URLs given by the user. See the upstream PR for details.

I pushed another commit which pulls in go-marathon 0.7.1 and contains the fix.

Please don't mind the now-wrong branch name. :-)

@containous/traefik Please take a look.

@timoreimann
Copy link
Contributor Author

Since @containous/traefik is only working for maintainers: Hailing @emile @SantoDE @errm @russ @dtomcej @vdemeester directly for review. :)

Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐸
I wish we would pin them to this hash to make sure another glide up somewhere else won't bring back the wrong hash but meh 🐮

Copy link
Collaborator

@SantoDE SantoDE left a comment

Choose a reason for hiding this comment

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

LGTM 👼

@@ -1,5 +1,5 @@
hash: b2ac93355c3f551a75216a800337cee9321f6c9a04a18ab1fa8d8152e89b7595
Copy link
Member

Choose a reason for hiding this comment

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

@timoreimann Not sure why the hash is unchanged here 🤔 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash is based on the content of the glide.yaml file. However, the manifest file did not change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular, go-marathon's version is specified as ^0.5.1 in glide.yaml, which is still valid given the latest version is 0.7.1.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, indeed ;)

@timoreimann
Copy link
Contributor Author

Squashed two latest commits and rebased.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @timoreimann :)

@emilevauge
Copy link
Member

emilevauge commented Feb 20, 2017

@vdemeester

I wish we would pin them to this hash to make sure another glide up somewhere else won't bring back the wrong hash but meh 🐮

We could! No?

@timoreimann
Copy link
Contributor Author

@vdemeester:

I wish we would pin them to this hash to make sure another glide up somewhere else won't bring back the wrong hash but meh

I could follow up with another PR to raise the minimum version to 0.7.1 if you like.

Brings github.com/gambol99/go-marathon version 0.7.1.
@timoreimann
Copy link
Contributor Author

@emilevauge @vdemeester @SantoDE Good to merge? Again, happy to bump the minimum go-marathon version either in this PR or a follow-up. Just let me know.

@vdemeester vdemeester merged commit a0a0bf0 into traefik:master Feb 21, 2017
@timoreimann timoreimann deleted the upgrade-go-marathon-to-v0.7.0 branch February 21, 2017 19:50
@ldez ldez modified the milestone: 1.3 May 19, 2017
@ldez ldez added kind/enhancement a new or improved feature. and removed status/3-needs-merge labels May 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants