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

Bump go-marathon dep #1524

Merged
merged 2 commits into from
May 3, 2017
Merged

Conversation

jangie
Copy link
Contributor

@jangie jangie commented May 2, 2017

Was: #1519 before I did something without thinking.

Description

Update go-marathon dependency. Addresses gambol99/go-marathon#274 , which is problematic for Traefik users who use Traefik against the Marathon provider, where the Marathon provider is protected by basic auth. At current with that setup, if a given Marathon instance goes out of service, Traefik will not be able to reconnect to that Marathon instance and will essentially have a no-longer-updated frontend/backend configuration. (Traefik will continue to service known routes successfully, but new deployments will not be tracked.)

This dependency update aims to resolve that situation.

As a note: I was unable to lock the docker/docker dependency, I suspect that there is something to do with how the coreos/etcd dependency is doing things that didn't allow the locking to work as expected.

Related to #1510

@ldez ldez added kind/bug/fix a bug fix area/provider/marathon priority/P1 need to be fixed in next release labels May 2, 2017
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.

SGTM

glide.lock Outdated
@@ -667,4 +728,61 @@ imports:
- tools/clientcmd/api
- tools/metrics
- transport
testImports: []
testImports:
Copy link
Contributor

Choose a reason for hiding this comment

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

These imports are for the integration tests. This means we might be able to remove the integration vendor folder that way.
cc @timoreimann @ldez

Copy link
Contributor

Choose a reason for hiding this comment

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

@jangie can you make a commit on top of this one that removes integration/glide.{yaml,lock} and integration/vendor to see if it's still green ? 👼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vdemeester just in case you missed it in the fog of war: I tried this, and did not receive a green. https://travis-ci.org/containous/traefik/jobs/227970015 is the respective job.

glide.lock Outdated
- registry/client/transport
- registry/storage/cache
- registry/storage/cache/memory
- uuid
- name: github.com/docker/docker
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all of thoses subpackages ? (anyway.. never really understood how glide handled them 😝)

@timoreimann
Copy link
Contributor

timoreimann commented May 2, 2017

@jangie out of curiosity, how did you update the dependencies? did you just run script/glide.sh (possibly multiple times)? Or did you do anything else?

@jangie
Copy link
Contributor Author

jangie commented May 2, 2017

@vdemeester will try!

@timoreimann i was using your approach but hadn't remembered that integration had their own glide configuration attached. The first few times I tried to run the glide update command, i ran into issues as there was a file and folder structure expected in docker/docker that wasn't available in the version we had locked it to. I will retry, taking into account the integration glide configurations.

@timoreimann
Copy link
Contributor

timoreimann commented May 2, 2017

@jangie if you use the "dirty" method, you should end up with a single (go-marathon) package getting updated per lock file only (besides the hash the timestamp, that is).

@jangie
Copy link
Contributor Author

jangie commented May 2, 2017

@timoreimann I've tried the process again, this time performing a glide pin against the integration, ensuring that its glide update -v finished cleanly, and then moving on to the main glide.yaml in case it was something in the integration configuration causing it to not be pinned. However, I'm still receiving the following (which is what caused me to bump the docker/docker version in the commit above):

[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.
[ERROR]	Error scanning github.com/docker/docker/cli/config/configfile: open /Users/jlee23/.gvm/glide/current/cache/src/https-github.com-docker-docker/cli/config/configfile: no such file or directory
[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.
[ERROR]	Error scanning github.com/docker/docker/cli/command/image/build: open /Users/jlee23/.gvm/glide/current/cache/src/https-github.com-docker-docker/cli/command/image/build: no such file or directory
[ERROR]	This error means the referenced package was not found.
[ERROR]	Missing file or directory errors usually occur when multiple packages
[ERROR]	share a common dependency and the first reference encountered by the scanner
[ERROR]	sets the version to one that does not contain a subpackage needed required
[ERROR]	by another package that uses the shared dependency. Try setting a
[ERROR]	version in your glide.yaml that works for all packages that share this
[ERROR]	dependency.

Looking at github.com/docker/docker at v1.13.0 as is specified in my glide.yaml, it indeed seems as though it does not have the path that glide is looking for; this path seems to exist only in the v17.0.x line. My glide.yaml also does seem to have versions for each node within the yaml file, so at least as far as I can tell, each dependency should be pinned.

Removing the integration/glide.lock and integration/glide.yaml files and performing the same steps, while locking docker/docker at v1.13.0, results in the same errors.

I will be making a separate commit to do what @vdemeester suggested to see how that goes shortly.

@timoreimann
Copy link
Contributor

@jangie make sure you don't call glide directly but use script/glide.sh. It makes sure unused packages are being removed.

@jangie
Copy link
Contributor Author

jangie commented May 2, 2017

@timoreimann for clarity's sake, am i to use script/glide.sh update -v during the dirty process then? the steps listed in the comment said glide update -v so that's what i had been using.

@timoreimann
Copy link
Contributor

@jangie I think I was a bit sloppy on the comment. It should be script/glide.sh all the way.

@jangie
Copy link
Contributor Author

jangie commented May 2, 2017

Ah ok, I'll try again then after this build completes, I'm wanting to see if at least the integration portion can be removed from the Travis build. [it did not :( working on redoing following your steps]

@jangie
Copy link
Contributor Author

jangie commented May 2, 2017

@timoreimann fwiw, what i did just now was a script/glide.sh update instead of the glide update -v; trying to add in the -v gave the script heartburn as it was an extraneous argument. hopefully this builds happy!

@jangie jangie force-pushed the update-dep-go-marathon branch 3 times, most recently from 5ca6c5f to abeefa0 Compare May 2, 2017 15:48
@ldez ldez removed the priority/P1 need to be fixed in next release label May 3, 2017
@ldez ldez added this to the 1.3 milestone May 3, 2017
@timoreimann
Copy link
Contributor

I just realized that go-marathon isn't even used for the integration tests: The current Marathon test suite consists of a single test, and that just fires up a Marathon compose file and checks if Traefik is running. Nothing interacts with Marathon at this point.

I think we can safely ignore the integration tests for now. #1406 will deal with improving them in the future.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM.

jangie added 2 commits May 3, 2017 11:43
attempt to remove glide from integration

glide trim

Revert "attempt to remove glide from integration"

This reverts commit c5b42b6cdebb44e730080a0cf20a871c11ef095b.
fix docker dependency

remove unneeded docker dependency files

further cleanup
@timoreimann
Copy link
Contributor

Fixes #1510.

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.

Thanks @jangie
LGTM

@emilevauge emilevauge merged commit dcc4d92 into traefik:master May 3, 2017
@ldez ldez changed the title [Marathon] Bump go-marathon dep Bump go-marathon dep May 25, 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.

5 participants