This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

How would you use this in a script? Seems like it would be more useful to run docker-inspect to return the mapping in a machine-readable format.

It could be useful for:

def app = docker.build 'foo/bar:${env.BUILD_ID}
def container = app.run '-P'
try {
  def ip = container.port 8080
  input "http://${ip}"
} finally {
  container.stop
}

it could be used the same way to trigger integration-tests with the port, with a curl or something like that.

So I looked at the docker-inspect. We could use it but the syntax would be

docker inspect --format='{{(index (index .NetworkSettings.Ports "8080/tcp") 0).HostIp}}:{{(index (index .NetworkSettings.Ports "8080/tcp") 0).HostPort}}' ${container.id}

to have the same output and we would still have to write it to a file and then def ip = readFile 'the-file'.

I just saw that my code was totally incorrect. I pushed a new one which is valid.

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick don't you think it worth include this in the plugin? Redirecting the output of the docker port command is more a hack than an integration, no?

LGTM
From a usage point of view, what's the benefit vs using sh 'docker port $id 8080' ?

The issue with the docker port command is that you need to write the output to a file, read the file to assign it to a variable, and then remove the file if you want to clean up. So

def container = docker.build('foo/bar:123').run('-P')
def ip = container.port 8080

is currently written as

def container = docker.build('foo/bar:123').run('-P')
sh "docker port ${container.id} 8080 > tmp-file"
def ip = readFile('tmp-file').trim()
sh 'rm tmp-file'

with the possibility that the tmp-file is existing and used for the build.

ok, then a better fix would be to get some descent support for returning value from a sh pipeline step, equivalent to bash foo = $(some command)

anyway, still LGTM as improvement to docker-pipeline DSL

returning value from a sh pipeline step

Yes, JENKINS-26133 is well known.

Anyway I must still be missing something fundamental. The newly added method merely runs docker port and leaves the output—which is formatted for human consumption—in the build log, where it cannot be used by the script. So this seems totally pointless.

If you have some actual use case, write a functional test proving your addition works.

formatted for human consumption

Well, at least that is what ports produces. port produces compact output. But again not in a way that the script could consume it.

Well, at least that is what ports produces. port produces compact output. But again not in a way that the script could consume it.

I agree for ports, we could remove it as not script-readable. However, for port I disagree. The output is 123.123.123.123:1234 which can be used to provide container entry point to reach HTTP API required for selenium tests or manual validation through an input step.

So, delete ports, and fix port to actually return a value, rather than printing something to the log (and write a test proving it).

🐝 but

Merge conflict in src/test/java/org/jenkinsci/plugins/docker/workflow/DockerDSLTest.java

I'm trying to resolve the conflict without adding useless (to this PR) commit in the branch

resolve the conflict without adding useless (to this PR) commit

Not sure what you mean. You need merely

git merge master

resolve, commit, and push. What is the problem?

And please avoid rebasing. It makes it impossible to see your incremental changes.

🐝

merging would have "imported" the commits on master here. Those commits would have nothing to do with the PR, when rebase just move the current branch to the new head of master. I understand it makes the incremental changes impossible to watch, but the commit graph in the end if much more easy to ready imo.

merging would have "imported" the commits on master here

No it does not. GitHub displays only those commits which are specific to the branch (not in the base branch).