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

[RHELC-1432] Copy instead of move when restoring a file #1122

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

r0x0d
Copy link
Member

@r0x0d r0x0d commented Feb 29, 2024

Moving a file back to its own directory is causing the file to lose the metadata and not letting the user enter in their system with SSH after the analysis is done.

Jira Issues: RHELC-1432

Checklist

  • PR has been tested manually in a VM (either author or reviewer)
  • Jira issue has been made public if possible
  • [RHELC-] is part of the PR title
  • GitHub label has been added to help with Release notes
  • PR title explains the change from the user's point of view
  • Code and tests are documented properly
  • The commits are squashed to as few commits as possible (without losing data)
  • When merged: Jira issue has been updated to Release Pending if relevant

Moving a file back to its own directory is causing the file to lose the
metadata and not letting the user enter in their system with SSH after
the analysis is done.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.30%. Comparing base (ede74b2) to head (a751c15).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1122   +/-   ##
=======================================
  Coverage   95.30%   95.30%           
=======================================
  Files          51       51           
  Lines        4644     4645    +1     
  Branches      822      822           
=======================================
+ Hits         4426     4427    +1     
  Misses        139      139           
  Partials       79       79           
Flag Coverage Δ
centos-linux-7 90.42% <100.00%> (+<0.01%) ⬆️
centos-linux-8 91.39% <100.00%> (+<0.01%) ⬆️
centos-linux-9 91.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bookwar bookwar added the tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. label Feb 29, 2024
@has-bot
Copy link
Member

has-bot commented Feb 29, 2024

/packit test --labels tier0


Comment generated by an automation.

@kokesak
Copy link
Member

kokesak commented Feb 29, 2024

/packit test --labels tier0

@@ -120,7 +120,8 @@ def restore(self, rollback=True):
return

try:
shutil.move(self._backup_path, self.filepath)
shutil.copy2(self._backup_path, self.filepath)
os.remove(self._backup_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's an exception raised by the shutil.copy2 (for example the target directory doesn't exist), the os.remove is not called.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but that's how we should have it. If something didn't go well with the copy we don't want to remove the backup

Copy link
Member

@danmyway danmyway left a comment

Choose a reason for hiding this comment

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

Looks good from QE POV.
Thank you for the fix @r0x0d !

Copy link
Member

@hosekadam hosekadam left a comment

Choose a reason for hiding this comment

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

Since the behavior of move is copy and then remove the original file, the change does in fact the same.

@r0x0d r0x0d enabled auto-merge (squash) March 1, 2024 13:43
@r0x0d r0x0d added the kind/bug-fix A bug has been fixed label Mar 1, 2024
@r0x0d r0x0d merged commit 9e3350b into oamg:main Mar 1, 2024
47 of 63 checks passed
@r0x0d r0x0d deleted the change-move-to-copy-in-backup branch March 1, 2024 13:44
@Venefilyn Venefilyn mentioned this pull request Mar 4, 2024
jochapma pushed a commit to jochapma/convert2rhel that referenced this pull request Mar 11, 2024
Moving a file back to its own directory is causing the file to lose the
metadata and not letting the user enter in their system with SSH after
the analysis is done.

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix A bug has been fixed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants