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

Remove endpoint protocol according to latest docker specific #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ikikko
Copy link
Member

@ikikko ikikko commented Sep 30, 2017

Problem

I couldn't docker.image("xxx").pull() with withRegistry from Amazon ECR when I had another job in which I used aws ecr get-login and docker pull with native awscli & docker cli.

Cause

This plugin updated ~/.docker/config.json in a wrong format. We didn't have a problem when we used only this plugin. However, we had a problem when there was another job with native awscli.

The config file ~/.docker/config.jsonis like a following.

{
  "auths":   {
    "1234567890.dkr.ecr.us-east-1.amazonaws.com":     {  --- (A)
      "auth": ... ,
      },
    "https://1234567890.dkr.ecr.us-east-1.amazonaws.com":     { --- (B)
      "auth": ... ,
      }
 ...
  }
}

A problem procedure was a following way.

  1. A job which uses aws ecr get-login
    1. Add auth entry (A) to config.json
  2. A pipeline which uses docker.withRegistry()
    1. Refresh auth entry (B) in config.json
    2. A auth entry (A) is used when docker pull in the pipeline above
    3. docker pullfails if a auth entry (A) is expired

Though I'm not 100% sure, according to https://github.com/moby/moby/pull/23100/files#diff-0ec52e4c7165f2740e01649dcef5867b , both docker client should uses hostname key (A), and (B) is for backward compatibility. However this plugin updated auth key (B).

@ikikko
Copy link
Member Author

ikikko commented Sep 30, 2017

There is no problem so far after I fixed this plugin and installed that to my Jenkins a week ago.

@jglick
Copy link
Member

jglick commented Oct 10, 2017

Same comment as #59: maybe right, maybe not, but anyway all this code would better be rewritten.

@jglick
Copy link
Member

jglick commented Feb 16, 2018

Likely unnecessary after #67, though could still apply to corner cases such as users of the Google container registry which cannot be handled by a simple docker login.

@oleg-nenashev oleg-nenashev requested a review from jglick May 13, 2019 08:28
@jglick jglick removed their request for review November 20, 2019 19:08
@jglick
Copy link
Member

jglick commented Nov 20, 2019

I would recommend just running something along the lines of

eval `aws ecr get-login --no-include-email`

from a sh step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants