Skip to content

feat(operations.files): Ensure file ops don't fail if source missing in prepare phase - #1915

Open
aktiur wants to merge 1 commit into
pyinfra-dev:3.xfrom
aktiur:file_ops_missing_sources
Open

feat(operations.files): Ensure file ops don't fail if source missing in prepare phase#1915
aktiur wants to merge 1 commit into
pyinfra-dev:3.xfrom
aktiur:file_ops_missing_sources

Conversation

@aktiur

@aktiur aktiur commented Aug 7, 2026

Copy link
Copy Markdown

Updated the move, copy and unarchive operations so that they won't raise an operation error when the source file (or target directory) is missing while in the prepare phase. Instead, they simply log an info message.

During the execution phase, the operations raise errors as before.

This commit is based on previous work by @vaz-ar in PR #1787.

All tests pass. However, the FakeState used by pyinfra-testing always has is_executing set to false. This means only the prepare phase code path is actually tested. On a similar note, I haven't seen a way to assert what is logged during the operation. How should I proceed to make the tests more complete?

@aktiur aktiur changed the title feat(operations.files): Ensure file ops don't fail if source missing feat(operations.files): Ensure file ops don't fail if source missing in prepare phase Aug 7, 2026
@wowi42 wowi42 added new feature operations Issues with operations. labels Aug 16, 2026

@wowi42 wowi42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

The core logic is solid. During the prepare phase, move, copy, and unarchive now log an info message and proceed when the source or destination directory doesn't exist yet, rather than raising OperationError. The execution phase still raises errors as before. The False vs None distinction in the fact return values is handled correctly: False (path exists but is wrong type) always raises, None (doesn't exist yet) is lenient during prepare.

Two small things:

First, the host.loglogger.info change in the line function and the import reordering of format_text are unrelated to this PR. Functionally identical, just a style change. Please split these out into a separate PR.

Second, move/invalid_dest.json was changed from testing null (dest doesn't exist) to false (dest is not a directory). Both are useful, but the "doesn't exist" prepare-phase path for move's dest is now untested. A follow-up fixture would close that gap — the copy equivalent is already covered by copy/invalid_dest.json.

On execution-phase testing: you're right that FakeState always has is_executing=False. The is_executing branches are simple enough to verify by inspection, so this isn't a blocker. If you want to add coverage later, a small patch to pyinfra-testing to allow setting is_executing on the fake state would be the cleanest approach.

@aktiur
aktiur force-pushed the file_ops_missing_sources branch from ddb2001 to 326a95e Compare August 18, 2026 10:07
@aktiur
aktiur force-pushed the file_ops_missing_sources branch from 326a95e to 7f69148 Compare August 18, 2026 10:11
@aktiur

aktiur commented Aug 18, 2026

Copy link
Copy Markdown
Author

Thanks for your review! I updated the PR to address your two remarks:

First, the host.loglogger.info change in the line function and the import reordering of format_text are unrelated to this PR. Functionally identical, just a style change. Please split these out into a separate PR.

My mistake. I originally used host.log in the prepare phase code path of the operations I modified, but realised I could not, because log is not defined on the FakeHost used during tests. I was not careful and/or too zealous when I did some find/replace to handle that. It's gone from the PR.

Second, move/invalid_dest.json was changed from testing null (dest doesn't exist) to false (dest is not a directory). Both are useful, but the "doesn't exist" prepare-phase path for move's dest is now untested. A follow-up fixture would close that gap — the copy equivalent is already covered by copy/invalid_dest.json.

I created two additional tests for both files.copy and files.move named missing_dest that cover the case where the destination directory is missing. The original tests (named invalid_test) both cover the case where the destination is not a directory.

On execution-phase testing: you're right that FakeState always has is_executing=False. The is_executing branches are simple enough to verify by inspection, so this isn't a blocker. If you want to add coverage later, a small patch to pyinfra-testing to allow setting is_executing on the fake state would be the cleanest approach.

I'm going to look into that.

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

Labels

new feature operations Issues with operations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants