#712 Infer registry URL from Template image name #807
Conversation
|
A fix might be needed upstream as the |
| String hostname = NameParser.resolveRepositoryName( | ||
| NameParser.parseRepositoryTag(getImage()).repos).hostname; | ||
| if(!hostname.startsWith("https://")) { | ||
| hostname = "https://" + hostname; |
There was a problem hiding this comment.
Are you 100% sure that https:// is the correct default here?
I know that docker registries can use https, and if one is authenticating then it's best to use an encrypted means of communicating, but it looks like the code here would prevent someone from using "http://hostname.domain" as this would be turned into "https://http://hostname.domain" which is unlikely to work at all.
I suspect that it'd be better to only prefix with https:// if there was no protocol specified at all ... but I'm not entirely sure of that here (I'm not in a position to experiment/evaluate right now either - current work pressures are such that my time for this is limited to "review, ask questions, push merge button and hope").
There was a problem hiding this comment.
I suspect that it'd be better to only prefix with https:// if there was no protocol specified at all ...
NameParser#resolveRepositoryName actually throw an exception if you provide the scheme in the image name. That is to say that http://hostname.domain/repo/name:tag is not a valid image name. So you cannot specify http://.
However NameParser#resolveRepositoryName use https://index.docker.io/v1/ if there no registry specified, hence the condition.
I agree that we could just do this if there is no protocol at all.
Are you 100% sure that https:// is the correct default here?
DockerRegistryEndpoint is also setting https:// for the scheme. The help says The url is built from registry:port into https://registry:port, the same way docker push does..
| @@ -567,7 +567,13 @@ public void setExtraDockerLabelsString(String extraDockerLabelsString) { | |||
|
|
|||
| public DockerRegistryEndpoint getRegistry() { | |||
| if (registry == null) { | |||
| registry = new DockerRegistryEndpoint(null, pullCredentialsId); | |||
| /* TODO: Use DockerRegistryEndpoint#fromImageName when JENKINS-63243 is fixed */ | |||
There was a problem hiding this comment.
Is there a PR for this issue yet?
There was a problem hiding this comment.
There was a problem hiding this comment.
That PR had to wait a long time, but it looks like that PR was merged in on 24 Aug, and went into docker-commons 1.20 onwards.
Could you update this PR to use the new code (i.e. resolve the TODO comment)?
pjdarton
added
the
enhancement
src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
Outdated
Show resolved
Hide resolved
Addresses #712. Infer the registry URL from image name using the org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint#fromImageName
Fixes #712