prometheus-community / ansible

Ansible Collection for Prometheus
https://prometheus-community.github.io/ansible/
Apache License 2.0
393 stars 133 forks source link

fix: mark download tasks as unchanged #433

Closed Frazew closed 1 month ago

Frazew commented 1 month ago

Summary

This PR suggests changing the behavior of the tasks downloading the binaries following the discussions on #431.

The main arguments for this are:

The main arguments against this are:

I'm of course biased towards the arguments in favor of this change, please feel free to argue against it!

gardar commented 1 month ago

Hard pass on this one from me, Ansible is idempotent by design and I don't see a reason why we should go against that by faking the changed status of tasks, it's only going to cause issues and confusion down the road.

* the tasks are executed locally, they do not affect the remote host, therefore having them mark the remote host as `changed` is unnecessarily confusing

* only the first host in the host loop will be `changed` because of that, which only adds to the confusion

* the task which we really care about to tell whether the remote host was changed or not is the `Propagate binaries` task

If you take a look at the output from the role execution you see that the task was delegated to localhost.

It is changing the localhost, it is creating new files that were not there before.

* it's not always possible to preserve the `_common_local_cache_path` folder between ansible runs: if I deploy an exporter on a host I expect to see no `changed` if I re-run the role several weeks later.

You can override the variables for the roles to point to a persistent directory rather than the default which is under /tmp. With that being said, please don't attempt to override the variable of the _common role - that won't work. Instead you should override the cache path variables of the respective roles.

You can even pre populate that dir and then you can fully bypass the download tasks.

The main arguments against this are:

  • you should be able to see when the download is actually being executed, and changed_when: false hides that completely

And if there is something wrong with your setup and you accidentally download the binaries over and over again without realizing it you are going to have a hard time realizing what's going on.

* this is a non-issue if you should properly cache `_common_local_cache_path` between runs to avoid repeated download requests

This is the correct solution, alternatively you can pre populate the dir with pre_tasks or something.

SuperQ commented 1 month ago

Thanks @gardar, I 100% agree with your assessment here.