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 go-marathon to dd6cbd4. #1800

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Jun 28, 2017

Fixes a problem with UnreachableStrategy being available now in two type-incompatible formats (object and string).

Fixes #1743.

@timoreimann
Copy link
Contributor Author

Missed to update one transitive dep. Doing that now...

@timoreimann timoreimann force-pushed the upgrade-go-marathon-to-dd6cbd4 branch from e94e34f to dc6882f Compare June 28, 2017 13:56
@ldez ldez added this to the 1.3 milestone Jun 28, 2017
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez
Copy link
Contributor

ldez commented Jun 28, 2017

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

you need to update glide.yml and the hash 😉

@@ -93,7 +93,6 @@ import:
- package: k8s.io/client-go
version: v2.0.0
- package: github.com/gambol99/go-marathon
version: d672c6fbb499596869d95146a26e7d0746c06c54
Copy link
Contributor

Choose a reason for hiding this comment

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

please add the hash instead remove it

Copy link
Contributor

@ldez ldez Jun 28, 2017

Choose a reason for hiding this comment

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

if you don't do that you produce side effect when other dependencies are added.

I have spend a lot of time to fix the lock and the yml to be stable.

Copy link
Contributor Author

@timoreimann timoreimann Jun 28, 2017

Choose a reason for hiding this comment

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

Not sure I understand... why do we need the hash with go-marathon in the glide manifest file when we don't specify one with the vast majority of other dependencies?

Semantically, we shouldn't have to add it as we are not after a specific version: When we upgrade go-marathon, we tend to look for the latest.

Copy link
Contributor

@ldez ldez Jun 28, 2017

Choose a reason for hiding this comment

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

It's hard to explain, sorry if I will misspoke.

Currently when you add a dependency, you don't need to pin version to yml (as we did before)

Glide do some "random" update when some version are not fixed.
I don't know exactly why, but I know Glide love the past and not the present...
Some dependencies don't update when you add a dependency but not all...

To be "safe", some dependencies must be fixed. (IMHO in the yml all dependencies must be fixed on version or hash)

I know this is not prefect but I don't want to do again +8 hours of hash analysis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, understood. FWIW, a number of glide problems can be resolved by clearing the cache up-front (i.e., running glide cc); but I know from experience that it doesn't always help.

I updated the YML file and hash. Hope it's okay now.

Fixes a problem with UnreachableStrategy being available now in two
type-incompatible formats (object and string).

We also upgrade the transitive dependency
github.com/donovanhide/eventsource.
@timoreimann timoreimann force-pushed the upgrade-go-marathon-to-dd6cbd4 branch from 44e8d99 to dcedb26 Compare June 28, 2017 21:53
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

And now ...

LGTM !!!!

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM

@ldez ldez merged commit 9fbe21c into traefik:v1.3 Jun 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants