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.

Sign up for GitHub

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

Jump to bottom

command/{cp, mv, rm}: add dry-run option #200

Closed
wants to merge 10 commits into from
Closed

command/{cp, mv, rm}: add dry-run option #200

wants to merge 10 commits into from

Conversation

fbarotov
Copy link
Contributor

Resolves #90

@sonmezonur
Copy link
Member

sonmezonur commented Jul 28, 2020 •
edited
Loading

Can we additionally add --dry-run option to run command?

s5cmd run --dry-run commandfile

Also, please update readme to introduce this new feature.

sonmezonur
sonmezonur reviewed Jul 29, 2020
View reviewed changes
Copy link
Member

@sonmezonur sonmezonur left a comment

Choose a reason for hiding this comment

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

💯 if we add one simple test for run command (with multiple remove, copy operations).

CHANGELOG.md Outdated Show resolved Hide resolved
igungor
igungor reviewed Aug 4, 2020
View reviewed changes
command/cp.go Outdated Show resolved Hide resolved
command/run.go Show resolved Hide resolved
igungor
igungor reviewed Aug 12, 2020
View reviewed changes
command/cp.go Show resolved Hide resolved
command/cp.go Show resolved Hide resolved
command/rm.go Outdated Show resolved Hide resolved
@igungor igungor changed the title command/{cp, mv, rm}: added dry-run option command/{cp, mv, rm}: add dry-run option Aug 12, 2020
igungor
igungor reviewed Aug 14, 2020
View reviewed changes
command/rm.go Outdated Show resolved Hide resolved
igungor
igungor reviewed Aug 14, 2020
View reviewed changes
e2e/cp_test.go Show resolved Hide resolved
e2e/cp_test.go Show resolved Hide resolved
e2e/run_test.go Outdated Show resolved Hide resolved
igungor
igungor requested changes Aug 14, 2020
View reviewed changes
Copy link
Member

@igungor igungor left a comment

Choose a reason for hiding this comment

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

Adding a review to avoid accidental merge. @fbarotov is gonna try to hide the dry-run logic into storage implementation.

@igungor
Copy link
Member

igungor commented Aug 19, 2020

#210 is the prefered implementation. Closing this one.

@igungor igungor closed this Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonmezonur sonmezonur sonmezonur approved these changes

@ilkinulas ilkinulas ilkinulas left review comments

@igungor igungor igungor requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add --dry-run option
4 participants
@fbarotov @sonmezonur @igungor @ilkinulas