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

Fix docker version specifier #1108

Merged
merged 2 commits into from
Feb 3, 2017

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Feb 2, 2017

This commit from PR #1067 introduced a wrong Docker version specifier: The major version is 1 and does not come with a leading v character. The PR corrects both.

To my surprise, the Docker build on Travis CI did not invalidate the cache when the originating PR ran. As far as I understand the documentation, this should have been the case. The problem wasn't caught during the Travis CI run since we set the DOCKER_VERSION environment variable in the Travis configuration file and pass it through as the build argument, thereby never invalidating the corresponding Docker cache layer since ARGs are invalidated only when their content changes. Running any Docker-requiring Make target locally, however, surfaces the issue. I'm not exactly sure how to detect such problems in the future.

Additionally, it wasn't super easy to spot the problem due to the chosen curl parameters. Hence, I piggybacked the following changes:

  • Added -f (--fail): It turns HTTP error response codes into non-zero exit codes, making curl fail early and properly. While the documentation mentions that there is supposed to be no output, we do see an error message. (I got a pretty clear curl: (22) The requested URL returned error: 403 Forbidden.)
  • Dropped -S (--show-error): It is only meaningful when used together with -s (--silent). We do not want to go silent but see the progress bar though.

- The download URL[1] does not contain a leading 'v'.
- The major version is 1.

[1] https://github.com/docker/docker/releases/tag/v1.10.3
- `-f` (`--fail`) turns HTTP error response codes into a non-zero exit
  code, making curl fail early and properly. While the documentation
  mentions that there is supposed to be no output, we do see an error
  message.
- `-S` (`--show-error`) is only meaningful when used together with `-s`
  (`--silent`). We do not want to go silent but see the progress bar
  though.
@timoreimann
Copy link
Contributor Author

@emilevauge please take a look.

@timoreimann
Copy link
Contributor Author

if desired, I can insert a commit that shows the problem followed by the fix.

@emilevauge
Copy link
Member

Indeed, I was also preparing a PR to fix that ;) thanks!

emilevauge referenced this pull request Feb 2, 2017
* Fix travis script

Signed-off-by: Emile Vauge <[email protected]>

* how do i pronounce this damn project

Signed-off-by: Emile Vauge <[email protected]>

* Remove unstable Docker 1.13 tests

Signed-off-by: Emile Vauge <[email protected]>
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 can't believe I missed that one 😂

@vdemeester vdemeester merged commit c9e78c4 into traefik:master Feb 3, 2017
@timoreimann timoreimann deleted the fix-docker-version-specifier branch February 3, 2017 10:13
jangie pushed a commit to jangie/traefik that referenced this pull request Feb 23, 2017
* Fix Docker version specifier.

- The download URL[1] does not contain a leading 'v'.
- The major version is 1.

[1] https://github.com/docker/docker/releases/tag/v1.10.3

* Drop -S and and -f in build.Dockerfile curl commands.

- `-f` (`--fail`) turns HTTP error response codes into a non-zero exit
  code, making curl fail early and properly. While the documentation
  mentions that there is supposed to be no output, we do see an error
  message.
- `-S` (`--show-error`) is only meaningful when used together with `-s`
  (`--silent`). We do not want to go silent but see the progress bar
  though.
@ldez ldez added this to the 1.2 milestone May 19, 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