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

fix: mark download tasks as unchanged #433

Closed

Conversation

Frazew
Copy link

@Frazew Frazew commented Oct 18, 2024

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 tasks are executed locally, they do not affect the remote host, therefore having them mark the remote host as changed is unnecessarily confusing
  • the tasks are prerequisites for the role anyway, you expect them to download something because it wouldn't make sense otherwise
  • 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
  • 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.

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
  • this is a non-issue if you should properly cache _common_local_cache_path between runs to avoid repeated download requests

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

Signed-off-by: François HORTA <fhorta@scaleway.com>
@gardar
Copy link
Member

gardar commented Oct 18, 2024

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.

@gardar gardar closed this Oct 18, 2024
@gardar gardar added the wontfix This will not be worked on label Oct 18, 2024
@SuperQ
Copy link
Contributor

SuperQ commented Oct 20, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment