We could do it with the --format template, but since it's JSON we might be better off with just using jq?

vs via jq:

Latter looks simpler to grok to me 🤔

well, jq could work, the comment tries actually explaining of the fallbacks:

• jq needs -r and // ""

  docker inspect container_with_bridge | jq '.[].NetworkSettings.Networks["bridge"].IPAddress'
  # "172.17.0.2"
  docker inspect container_with_bridge | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress'
  # 172.17.0.2
  docker inspect container_without_bridge | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress'
  # null
  docker inspect container_without_bridge | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress // ""'
  # 

  and that would need the adequate comments for explaining the fallback (as i can understand that most people would find that hard to read). i just documented the cases i could think of were the new codes output is the same as the old code with docker < 29.

Should i also change _container_is_running to jq?

And also introducing a pipe would mean we should check the error handling:

docker inspect unknown_container | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress // ""' || echo "FAIL"
#
#error: no such object: unknown_container
docker inspect --format '{{with (index .NetworkSettings "IPAddress")}}{{.}}{{else}}{{with (index .NetworkSettings.Networks "bridge")}}{{.IPAddress}}{{end}}{{end}}' unknown_container || echo "FAIL"
#
#error: no such object: unknown_container
#FAIL

Regarding the jq fallback to empty with // "", while the reasoning is good our tests don't seem to use such a fallback when encountering a failure.

# With blank line included (stdout):
$ docker inspect --format '{{ .NetworkSettings.IPAddress }}'

Error: No such object: unknown_container

I'm not sure why your output example differs there with all lowercase 🤔 (Oh, just compared to v29+ where it is lowercase, prior to that it was as my output shows)

that would need the adequate comments for explaining the fallback (as i can understand that most people would find that hard to read).

I'd agree if we had the jq fallback you suggest it would need clarification.

From the looks of it the blank value to stdout probably isn't meaningful to our tests (fail2ban.bats + postscreen.bats that use this utility method), they'd still fail as your shared error output demonstrated with stderr being reported instead.

And also introducing a pipe would mean we should check the error handling:

set -o pipefail will handle that:

$ set -o pipefail
$ docker inspect unknown_container | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress // ""' || echo "FAIL"
error: no such object: unknown_container

FAIL
$ echo $?
1

That said there's quite a bit of discouragement online for using pipefail broadly/globally, so that's a bit of a bummer.

jq does have an option to output an exit code for invalid input though:

-e / --exit-status: Sets the exit status of jq to

• 0 if the last output values was neither false nor null
• 1 if the last output value was either false or null
• 4 if no valid result was ever produced.

$ docker inspect unknown_container | jq -r --exit-status '.[].NetworkSettings.Networks["bridge"].IPAddress'
error: no such object: unknown_container
$ echo $?
4

Should i also change _container_is_running() to jq?

I don't think that's necessary other than for consistency. My main gripe was the template syntax for docker inspect --format is less intuitive vs the JQ equivalent:

docker inspect --format '{{with (index .NetworkSettings.Networks "bridge")}}{{.IPAddress}}{{end}}' "${TARGET_CONTAINER_NAME}"

vs

docker inspect "${TARGET_CONTAINER_NAME}" | jq -r --exit-status '.[].NetworkSettings.Networks["bridge"].IPAddress'

At least regarding the response is an array of objects, with some nested field access. In this case with jq I've added a bit of extra complexity for the bridge key since it won't work well if there's a hypen in the network name should this change in future (I assume that might be a similar case with your template equivalent syntax having a simpler form?), the jq query could just as easily be .[].NetworkSettings.Networks.bridge.IPAddress.

_container_is_running() is simple enough to grok:

vs

[[ $(docker inspect "${TARGET_CONTAINER_NAME}" | jq -re '.[].State.Running') == 'true' ]]

After reading through the changes and this discussion, I am in favor of not using jq but keeping the changes as they are. They are mostly similar in quality IMHO but what jq gains in terms of lower complexity for its query it loses IMO in terms of error handling and cases with "" etc.

I think the query that this PR introduces is easy enough to understand.

what jq gains in terms of lower complexity for its query it loses IMO in terms of error handling and cases with "" etc.

There is no need to emit a blank output, the default null is fine since the error still can be output via --exit-status (causing the test to fail, while the earlier docker inspect stderr should be captured for the error message).

I am in favor of not using jq but keeping the changes as they are.

I disagree with "changes as they are", but I'll compromise.

We do not need the much longer docker inspect proposed here with two branches based on field detection when the latter is sufficient.

Regarding the jq fallback to empty with // "", while the reasoning is good our tests don't seem to use such a fallback when encountering a failure.

# With blank line included (stdout):
$ docker inspect --format '{{ .NetworkSettings.IPAddress }}'

Error: No such object: unknown_container

I'm not sure why your output example differs there with all lowercase 🤔 (Oh, just compared to v29+ where it is lowercase, prior to that it was as my output shows)

that would need the adequate comments for explaining the fallback (as i can understand that most people would find that hard to read).

I'd agree if we had the jq fallback you suggest it would need clarification.

From the looks of it the blank value to stdout probably isn't meaningful to our tests (fail2ban.bats + postscreen.bats that use this utility method), they'd still fail as your shared error output demonstrated with stderr being reported instead.

And also introducing a pipe would mean we should check the error handling:

set -o pipefail will handle that:

$ set -o pipefail
$ docker inspect unknown_container | jq -r '.[].NetworkSettings.Networks["bridge"].IPAddress // ""' || echo "FAIL"
error: no such object: unknown_container

FAIL
$ echo $?
1

That said there's quite a bit of discouragement online for using pipefail broadly/globally, so that's a bit of a bummer.

jq does have an option to output an exit code for invalid input though:

-e / --exit-status: Sets the exit status of jq to

• 0 if the last output values was neither false nor null
• 1 if the last output value was either false or null
• 4 if no valid result was ever produced.

$ docker inspect unknown_container | jq -r --exit-status '.[].NetworkSettings.Networks["bridge"].IPAddress'
error: no such object: unknown_container
$ echo $?
4

Should i also change _container_is_running() to jq?

I don't think that's necessary other than for consistency. My main gripe was the template syntax for docker inspect --format is less intuitive vs the JQ equivalent:

docker inspect --format '{{with (index .NetworkSettings.Networks "bridge")}}{{.IPAddress}}{{end}}' "${TARGET_CONTAINER_NAME}"

vs

docker inspect "${TARGET_CONTAINER_NAME}" | jq -r --exit-status '.[].NetworkSettings.Networks["bridge"].IPAddress'

At least regarding the response is an array of objects, with some nested field access. In this case with jq I've added a bit of extra complexity for the bridge key since it won't work well if there's a hypen in the network name should this change in future (I assume that might be a similar case with your template equivalent syntax having a simpler form?), the jq query could just as easily be .[].NetworkSettings.Networks.bridge.IPAddress.

_container_is_running() is simple enough to grok:

vs

[[ $(docker inspect "${TARGET_CONTAINER_NAME}" | jq -re '.[].State.Running') == 'true' ]]