Skip to content

Conversation

EricDriussi
Copy link

Adds copy operation suggested in #1121. Any and all feedback is welcome!

  • Pull request is based on the default branch (3.x at this time)
  • Pull request includes tests for any new/updated operations/facts
  • Pull request includes documentation for any new/updated operations/facts
  • Tests pass (see scripts/dev-test.sh)
  • Type checking & code style passes (see scripts/dev-lint.sh)

dest_file_path = os.path.join(dest, os.path.basename(src))
dest_file_exists = host.get_fact(File, dest_file_path)
if dest_file_exists and not overwrite:
raise OperationError(f"dest {dest_file_path} already exists and `overwrite` is unset")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this raise an OperationError. For copy() to be idempotent, the destination file existing, should just be a noop right? Alternatively, you could compare hashes to see if the src file has changed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed a noop would be a better fit here, although I still feel that an error should be raised in case dest_file_path already exists and is different from src. So maybe something like this?

    if dest_file_exists and not overwrite:
        if _file_equal(src, dest_file_path):
            host.noop(f"{dest_file_path} already exists")
        else:
            raise OperationError(f"{dest_file_path} already exists and is different than src")

Or maybe there's a better way to handle that case?

Also, in the snippet I use the _file_equal function, but that seems to be intended for local-remote hash comparisons.
Are you aware of any functions that might better fit this use case? Should I just create a new internal function based on this one for remote-remote hash comparison (what could be a nice name for it?)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants