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

Lowercase repository names (+ warnings if happens) #423

Closed
rhuss opened this issue Mar 30, 2016 · 10 comments
Closed

Lowercase repository names (+ warnings if happens) #423

rhuss opened this issue Mar 30, 2016 · 10 comments
Labels
Milestone

Comments

@rhuss
Copy link
Collaborator

rhuss commented Mar 30, 2016

Its not the tag but the name which needs to be sanitized. See comments below.

When tags are given, either as part of the image name or as extra tag, they should be sanitized to conform to the Docker syntax. I.e. no hyphens - are allowed and they should be translated to e.g. _.

Alternatively throw an error early. Currently the error message is quite confusing.

See #419 for an example.

@Gengar003
Copy link

As far as I can tell, a Hyphen (the character produced by pressing the key to the right of the "0" key on the top row of a standard US keyboard; unicode HYPHEN-MINUS) is totally valid in a Docker tag and in a Docker image name.

This character is frequently erroneously referred to as a "dash." Hyphens are the standard separator between Maven version numbers and their qualifiers.

Please do not update the docker-maven-plugin to begin translating valid characters in tag names into other characters.

Hyphens in Tags

  1. Docker says hyphens are allowed, just not as the first character: https://github.com/docker/distribution/blob/master/reference/regexp.go#L37
  2. Multiple images on Dockerhub use hyphens in their tags, such as busybox and nginx

Hypens in Image Names

  1. Docker says hyphens are allowed, just not as the first character: https://github.com/docker/distribution/blob/master/reference/regexp.go#L15-#L20
  2. Multiple images in Dockerhub have hyphens in their name, such as ubuntu-debootstrap and buildpack-deps

This Doesn't Even Fix It

I cloned the "reproduction" listed in #419 and changed all of the hypens - in the Maven versions to underscores _ (which causes 1.0_SNAPSHOT to be used as a tag instead of 1.0-SNAPSHOT), I got the exact same failure (the broken pipe as described in #419) as before.

My environment:

maven

/tmp/docker-maven-issue-broken-pipe (master) $ mvn --version
Apache Maven 3.3.3 (7994120775791599e205a5524ec3e0dfe41d4a06; 2015-04-22T06:57:37-05:00)
Maven home: /usr/local/Cellar/maven/3.3.3/libexec
Java version: 1.8.0_60, vendor: Oracle Corporation
Java home: /Library/Java/JavaVirtualMachines/jdk1.8.0_60.jdk/Contents/Home/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "10.11.3", arch: "x86_64", family: "mac"

docker

/tmp/docker-maven-issue-broken-pipe (master) $ docker-machine --version
docker-machine version 0.6.0, build e27fb87
/tmp/docker-maven-issue-broken-pipe (master) $ docker --version
Docker version 1.10.3, build 20f81dd

d-m-p

/tmp/docker-maven-issue-broken-pipe (master) $ mvn dependency:resolve-plugins | grep docker-maven
[INFO] Plugin Resolved: docker-maven-plugin-0.14.2.jar

But Please Sanitize Correctly

_I do not think hyphens, a valid character in both Docker tags and Docker image names, were the cause of issue #419. Please do not remove our ability to use this character in tags and image names with the docker-maven-plugin._

But sanitization would still be a good idea - though IMO the d-m-p should absolutely print a warning (or perhaps even fail unless sanitization is optionally enabled) if it is changing the output from what the user specified, to something else.

Have I grievously misunderstood what's going on here?

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

Thanks for the clarification. So I'm not really sure what is going on in #419. That's what I have in server log when I use a - dash:

time="2016-03-30T15:29:12.166435393Z" level=debug msg="POST /v1.18/build?t=vertx-acme-example%2FA%3A1.0-SNAPSHOT&nocache=0&forcerm=1"
time="2016-03-30T15:29:12.166494218Z" level=error msg="Handler for POST /v1.18/build returned error: Error parsing reference: \"vertx-acme-example/A:1.0-S
NAPSHOT\" is not a valid repository/tag"

But you are right, I get the same with _. (I could swear it worked this morning without hyphens.

So I'm still puzzled, the docker daemon used is 1.10.1

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

I guess its because of the username which needs to match [a-zA-Z0-9]+

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

nope, that's not. still puzzled, will reopen #419

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

Its that no capital letters are allowed. Strange, didn't know that.

What do you think, should we automatically lowercase stuff (+printing a warning) or break with an error ?

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

See also docker/compose#1416

@rhuss rhuss changed the title Sanitize Tags Lowercase repository names (+ warnings if happens) Mar 30, 2016
@Gengar003
Copy link

What do you think, should we automatically lowercase stuff (+printing a warning) or break with an error ?

My personal preference would be the following strategy towards sanitization:

  1. Add a new option which defaults to false; autoSanitizeNames (or some other, better name; naming is hard.)
  2. If autoSanitizeNames, then automatically sanitize/normalize the image and tag names so that they are guaranteed to be accepted by a valid Docker registry.
  3. If not autoSanitizeNames, and if a requested image name or tag name is invalid, fail the build with an ERROR (MojoExecutionException) and a helpful message indicating which name(s) weren't valid and that autoSanitizeNames can be enabled to automatically fix problems like this.

The overarching theme here is that the software shouldn't do something different than from what the user asked and call it a success. If a user instruments the plugin to push the A:some-%$#@tAG image, and the d-m-p silently pushes a:some-tag instead, the user will have a confusing, bad experience.

@rhuss
Copy link
Collaborator Author

rhuss commented Mar 30, 2016

Ok, so maybe we simply start with the error and add the autoSanitize feature when there is a compelling use case. For the moment, changing the image name manually is easy enough imo.

@cescoffier
Copy link

:+1 !

rhuss added a commit that referenced this issue Mar 31, 2016
Could be that validation needs to be performed at various places. Alternatively: Switch to ImageName everywhere instead of plain strings.
@rhuss rhuss added the fixed label Apr 1, 2016
@rhuss rhuss added this to the 0.15.0 milestone Apr 1, 2016
@rhuss rhuss closed this as completed Apr 27, 2016
rhuss added a commit that referenced this issue Sep 27, 2016
* Added property handler support
* Added documentation
* Changed from Properties to Map<String, String> since this fits better for the property handler
* Added a test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants