Allow healthcheck override without test option

[x4rd0o1Vtx]

SUMMARY

• Use case

Change healthcheck container options while keeping original image test.

ex:

healthcheck:
  interval: 1m
  timeout: 10s
  start_period: 30s
  retries: 3      

• Actual behavior

"Healthcheck" {
  "Test": ["NONE"]
}

• New behavior

"Healthcheck" {
  "Test": <internal docker image test>,
  "Interval": 60000000000,
  "Timeout": 10000000000,
  "StartPeriod": 30000000000,
  "Retries": 3
}

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

docker_api

[felixfontein]

Thanks for your contribution! Unfortunately this is a breaking change.

Maybe this could be done instead by adding a new option keep_image_test (type boolean, default false), which when set to true ignores the test value?

[x4rd0o1Vtx]

Thanks to you for maintaining this module! Indeed, I realised that few moment later after creating this PR...

I like this way to keep closest to docker cli behavior and this allow to do both keeping original test image by default or explicitly set ["NONE"] to unset it. Moreover, I saw there is only one rarely case to get the breaking change.

• Still keep the same behavior

healthcheck: {}

healthcheck:
  test: "" (or false, {}, [], omit)

=> "Healthcheck" {"Test": ["NONE"]}

• But not this one

healthcheck:
  test: "" (or false, {}, [], omit)
  interval: 1m

=> "Healthcheck" {"Test": ["NONE"]} (actual)
=> "Healthcheck" {"Test": None} (new)

This makes no sense to me because why adding test options like interval if you just want to unset the image test. But maybe this can result from conditional on test option follow by other options. So in this particular case, user should explicitly set ["NONE"] to test option to keep the actual behavior.

To handle this, maybe first reduce the breaking area to the omit case only, like I try in this commit by forwarding the real test value. (not 100% sure of potential side effect)

And if it's not enougth, maybe a mode or cli_compatible option could be a good way to keep the actual behavior and allow a breaking change on it on the future if you wish.

What do you think ?

[x4rd0o1Vtx]

This seems to have unexpected side effect based on some failing tests, I'll wait your feedback on my previous comment before going deep on it.

[felixfontein]

This makes no sense to me because why adding test options like interval if you just want to unset the image test. But maybe this can result from conditional on test option follow by other options. So in this particular case, user should explicitly set ["NONE"] to test option to keep the actual behavior.

To handle this, maybe first reduce the breaking area to the omit case only, like I try in this commit by forwarding the real test value. (not 100% sure of potential side effect)

I agree that it doesn't make sense, but it's still a breaking change of a behavior that has been this way for quite a few years. Someone might rely on this, and suddenly their deployments change (potentially in a fatal way).

Such changes need to come with a longer deprecation period where warnings are shown to the users. Since nobody wants to wait years to be able to use the new behavior, the best course of action is usually to add a new option to control this behavior which defaults to the current behavior, then eventually deprecate the default behavior (showing deprecation warnings in situation this results in a change), and eventually change the default.

And if it's not enougth, maybe a mode or cli_compatible option could be a good way to keep the actual behavior and allow a breaking change on it on the future if you wish.

Such an option would be best. A general cli_compatible option is not a good idea for this, since that behavior will also change over time when other incompatibilities are found. (And mode is way too generic, and usually means mode of a file.)

Having something inside the healthcheck dictionary is probably best, maybe an option called test_cli_compatible? That should also avoid potential collisions with things Docker adds, and it makes clear it is both about test and compatbility with the CLI.

[x4rd0o1Vtx]

Thanks for taking the time to explain why this way is still not possible and I understand better when you immediately suggested adding a new option. I thought there were special cases especially when this one seems to me very closer to a bugfix, even the documentation example may suggest that you absolutely should set ["NONE"] to the test option to remove the container healthcheck and that omitting this value should not have any effect.

- name: Remove healthcheck from container
  community.docker.docker_container:
    name: nginx-proxy
    image: nginx:1.13
    state: started
    healthcheck:
      # The "NONE" check needs to be specified
      test: ["NONE"]

In reality, all of these unset the healtcheck:

healthcheck: {}

healthcheck:
  test: "" (or false, {}, [], omit)

However I'm not doing this for myself but for the community and you know better than me what's best for the project so I'm totally fine to add a test_cli_compatible option.

Last question before making the changes, should I keep only the initial commit behavior or it's better to reduce to the omit case only like my second commit did ?

• First commit behavior

healthcheck:
  test_cli_compatible: true
  test: "" (or false, {}, [], omit)
  interval: 1m

=> "Healthcheck" {"Test": None}

• Second commit behavior

healthcheck:
  test_cli_compatible: true
  test: "" (or false, {}, [])
  interval: 1m

=> "Healthcheck" {"Test": ["NONE"]} (still keep the actual behavior)

healthcheck:
  test_cli_compatible: true
  test: "{{ omit }}"
  interval: 1m

=> "Healthcheck" {"Test": None}

[felixfontein]

I would use the second commit's behavior. If the user explicitly provides a value for test, it should override the image's value. (Unless test is not provided, or equivalently, if {{ omit }} is provided.)

[x4rd0o1Vtx]

Seems good for me but not sure about some failing tests, could you review this again please. Thanks in advance for your time.

[felixfontein]

If you want you can also add a similar change to the docker_swarm_service module. But maybe let's get this merged first so it's definitely part of the next release (which happens this weekend) :)

[felixfontein]

@x4rd0o1Vtx thank you very much for your contribution!

[x4rd0o1Vtx]

My pleasure and thanks for your time again !