Fix bug templating subdomain static dirs via Ansible module#244
Fix bug templating subdomain static dirs via Ansible module#244jdavcs merged 4 commits intogalaxyproject:mainfrom
Conversation
If the target path does not exist, `dst_stat = dst_path.lstat()` leads to an error `FileNotFoundError: [Errno 2] No such file or directory: ...`. Rearrange the line `dst_stat = dst_path.lstat()` and add a check based on `dst_path.exists()` for the case where `src_path.is_dir() == False` and `dst_path.exists() == False`; for example when the source is a file and the target does not exist.
…dirs The function `compare_permissions()` is meant to compare permissions, ownership and type (file or directory). But only if both paths exist. The case in which files exist only in the source directory is handled by `filecmp.dircmp().left_only` within `run_module()`.
`filecmp.dircmp()` does not compare directories recursively. To detect if subdomain static dirs contents have changed, use a new function `compare_dirs()` that runs `filecmp.dircmp()` recursively.
|
@jdavcs this is a follow-up for #232. The first error was detected because it was showing up on the CI, but I noticed also the second while fixing the former. It's surprising that it went under the radar for so long, especially considering it severly breaks the functionality (directories are only merged if a change is detected). |
Apply the bugfixes introduced in galaxyproject/ansible-galaxy#244 related to the deployment of subdomain static dirs.
jdavcs
left a comment
jdavcs
left a comment
There was a problem hiding this comment.
Great catch!
Works for me. I have just one request. It was not obvious to me what compare_dirs returns. I expected the opposite: True if there is no difference - i.e., similar to how filecmp.cmp() works. I don't switching to logic makes sense here since it's not a straightforward comparison like the cmp() function. However, maybe change the function name to something more explicit? (like dirs_have_changed?) Or, maybe even better, specify in the function docstring what it returns. Either one is fine - as long as it's obvious what it returns.
|
Thanks for creating the release! |
|
@kysrpex new release imported to ansible galaxy. (I saw you updated your infrastructure playbook) |
2 participants
This PR fixes two problems.
FileNotFoundErrorwhen a directory doesn't exist in the target subdomain static dirIf the target path does not exist,
dst_stat = dst_path.lstat()leads to an errorFileNotFoundError: [Errno 2] No such file or directory: ....Rearrange the line
dst_stat = dst_path.lstat()and add a check based ondst_path.exists()for the case wheresrc_path.is_dir() == Falseanddst_path.exists() == False; for example when the source is a file and the target does not exist.Given that we are not dealing with file types other than symlinks, directories and regular files, that should be enough.
Directory comparisons are not recursive.
filecmp.dircmp()does not compare directories recursively. To detect if subdomain static dirs contents have changed, use a new functioncompare_dirs()that runsfilecmp.dircmp()recursively.