fix: Better support parallel execution by allowing wget to skip files… #33
Conversation
… if they exist rather than fail
|
Would this update the package with a newer version if a newer version exists? For the people who might be using the module outside of a container or something that doesn't get cleared down with every run |
This would be no different to the current behaviour anyway since it checks it exists and doesn't download it. This just fixes the issue where the file appears after the check due to the parallel execution |
| @@ -10,7 +10,7 @@ resource "null_resource" "download" { | |||
| } | |||
|
|
|||
| provisioner "local-exec" { | |||
| command = "test -f ${local.package_directory}/${local.package_filename} || (mkdir -p ${local.package_directory} && wget -P ${local.package_directory} ${var.package_url})" | |||
| command = "test -f ${local.package_directory}/${local.package_filename} || (mkdir -p ${local.package_directory} && wget --no-clobber -P ${local.package_directory} ${var.package_url})" | |||
There was a problem hiding this comment.
You can now remove the call to test -f
There was a problem hiding this comment.
We could, but wouldn't it then download the file unnecessarily if it already existed? But yes, the parallel execution is a real edge case and most of the time it will be running in CI where the file won't exist so I think it is safe to remove and satisfies any concerns that older version will hang around
There was a problem hiding this comment.
The --no-clobber would prevent the file being unnecessarily downloaded
There was a problem hiding this comment.
Doh, yes you are right. I was thinking for some reason that it just prevented wget failing if it already existed, which is what I was originally looking to achieve.
There was a problem hiding this comment.
I don't really understand why the problem was consistently reproducible, because the test -f should usually prevent wget from downloading the file if it already exists. I would have thought it's pretty unlikely for one thread to start downloading the file while another thread is in-between the test -f and wget commands. Using --no-clobber would presumably prevent the problem completely though (depending on how it's implemented in wget).
There was a problem hiding this comment.
If TF runs this step in parallel the tests would all run and return false and then wget would run and the first one to download wins and the rest fail. If wget downloads to a temporary file the test won't pick it up until it is has completed (after enough time for all the tests to run). If wget creates the file immediately then yes, it does seem an unlikely coincidence. Would need to see the full TF output to better gauge the order things run in
This is a valid concern, as this is a public repo and we don't know whether people are always using it inside a fresh environment. Maybe we should append |
… if they exist rather than fail