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

Add dir support to kando with kopia location #1028

Merged
merged 5 commits into from
Jul 27, 2021
Merged

Conversation

PrasadG193
Copy link
Contributor

@PrasadG193 PrasadG193 commented Jun 18, 2021

Signed-off-by: Prasad Ghangal prasad.ghangal@gmail.com

Change Overview

This approach has the following limitations:

  1. The backup performed with stdin as a source, can be restored ONLY as stdout target
  2. The backup performed with file/dir path as a source, can be restored ONLY as file/dir path as target

Please see the test plan for the details.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Test cases:

  1. Backup file with stdin as source, restore file with stdout as target - PASS
  2. Backup file with stdin as source, restore file with filepath as target - FAIL
  3. Backup file with filepath as source, restore file with filepath as target - PASS
  4. Backup file with filepath as source, restore file with stdout as target - FAIL
  5. Backup dir with filepath as source, restore file with stdout as target and --path arg with same value as source filepath - PASS
  6. Backup dir with dirpath as source, restore dir with dirpath as target - PASS

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
@PrasadG193 PrasadG193 requested a review from pavannd1 June 18, 2021 12:41
@PrasadG193 PrasadG193 changed the title [WIP] Add dir support to kando Add dir support to kando with kopia location Jul 22, 2021
@PrasadG193 PrasadG193 marked this pull request as ready for review July 22, 2021 09:42
@pavannd1
Copy link
Contributor

pavannd1 commented Jul 27, 2021

I think test case 2 and 4 should work if the target path is same as the backup file path. Please try it out.

pkg/kopia/stream.go Show resolved Hide resolved
pkg/kopia/stream.go Show resolved Hide resolved
@PrasadG193
Copy link
Contributor Author

I think test case 2 and 4 should work if the target path is same as the backup file path. Please try it out.

@pavannd1 it works on master where we create Reader/Writer from file and pass it to stream handling. But since we have added separate handling for files and streaming data, they are no longer compatible.

Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
Signed-off-by: Prasad Ghangal <prasad.ghangal@gmail.com>
Comment on lines +245 to +251
// TODO: Do we want to keep this flags configurable?
output := &restore.FilesystemOutput{
TargetPath: p,
OverwriteDirectories: true,
OverwriteFiles: true,
OverwriteSymlinks: true,
IgnorePermissionErrors: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavannd1 do we want to have these flags configurable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay for now. We can improve later if the requirement comes up.

@PrasadG193 PrasadG193 requested a review from pavannd1 July 27, 2021 09:42
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

LGTM

@pavannd1 pavannd1 added the kueue label Jul 27, 2021
@mergify mergify bot merged commit 3632575 into master Jul 27, 2021
@mergify mergify bot deleted the kando-file-support branch July 27, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants