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

Expandable variables in DockerRegistry field #49

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

vadzay
Copy link
Contributor

@vadzay vadzay commented Oct 7, 2016

Hello!

I've noticed, that expandable variables do not work as expected if I use them in "Docker registry URL" field.
Everything works as expected, except for .dockercfg file created by Docker Commons Plugin.
If I use a variable in that field, file looks like this:

{ "https://${REGISTRY}":   {
    "auth": "<some base64 symbols>",
    "email": "<my_email@somewhere"
}}

As long as Docker Commons Plugin authors do not want to add yet another dependency, even this common, as TokenMacro Plugin, I've made a hack that allows me to use variables and expand them.

I tested it on my system and it seems to work OK.

I also had to add MacroEvaluationException to several methods' "throws", as long as executeCmd() now throws it.

I understand that this looks more like dirty hack, than a good code, but this is the only way I could produce to solve the problem.

@Vlatombe
Copy link
Member

Vlatombe commented Oct 7, 2016

did you ask Docker Commons author whether they were enclined to add integration with Token Macro? seems like a better way to me

@vadzay
Copy link
Contributor Author

vadzay commented Oct 28, 2016

Well, it looks like docker-build-publish should implement it rather than docker-commons.

Somewhere on the Net i found a post from docker-commons maintainer, saying that they provide something like a "library", rather than a regular plugin. I would like to post a link here, but could not find it, sorry.

Anyway, if they integrate with tokenMacro, they will have two options of handling MacroEvaluationException:

  1. try-catch every expand call and either throw their own exception or somehow handle invalid input data
  2. add MacroEvaluationException to "throws", and ask everyone using this plugin's code to rewrite their code

Anyway, it looks bad for them, I don't think that they will ever agree.

Also I think that as long as they provide a "library", it's docker-build-publish's responsibility to provide correct data to it's methods.

@antoinetran
Copy link

Hi, I agreed this should be docker build publish responsibility to expand the registry variable, the same way it already expands docker image name or docker build address.

As a workaround, for now, I expand it by putting the registry URL into the image name... Not very clean, but I have no choice.

As for the proper way to implement this expansion, maybe it should be set in the setRegistry()? Like this:

    public void setRegistry(DockerRegistryEndpoint registry) {
        this.registry = expandAll(registry);
    }

@rsandell rsandell merged commit 9d4eb2c into jenkinsci:master Aug 23, 2022
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