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 --replace_out option to bulk command. #3089

Merged
merged 4 commits into from
Mar 4, 2019
Merged

Conversation

codexnull
Copy link
Contributor

@codexnull codexnull commented Mar 4, 2019

Don't blindly replace the bulk out directory if it already exists and contains data. Fixes #3034.


This change is Reviewable

@codexnull codexnull requested a review from a team March 4, 2019 06:07
x/file.go Outdated
@@ -92,3 +93,30 @@ func FindDataFiles(str string, ext []string) []string {

return list
}

// IsMissingOrEmptyDir returns true if the path either does not exist or is a directory that is empty.

Choose a reason for hiding this comment

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

line is 102 characters (from lll)

// Make sure it's OK to create or replace the directory specified with the --out option.
// It is always OK to create or replace the default output directory.
if opt.OutDir != defaultOutDir && !x.IsMissingOrEmptyDir(opt.OutDir) && !opt.ReplaceOutDir {
fmt.Fprintf(os.Stderr, "Output directory exists and is not empty." +

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

// Make sure it's OK to create or replace the directory specified with the --out option.
// It is always OK to create or replace the default output directory.
if opt.OutDir != defaultOutDir && !x.IsMissingOrEmptyDir(opt.OutDir) && !opt.ReplaceOutDir {
fmt.Fprintf(os.Stderr, "Output directory exists and is not empty." +

Choose a reason for hiding this comment

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

File is not goimports-ed (from goimports)

Copy link

@gitlw gitlw left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @codexnull)


x/file.go, line 105 at r3 (raw file):

	}

	x.Check(err)

For better re-usability, I think it's better to return the err to the caller than crashing the program.
Except this, the PR looks good to me.

Copy link
Contributor Author

@codexnull codexnull left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @codexnull and @gitlw)


x/file.go, line 105 at r3 (raw file):

Previously, gitlw (Lucas Wang) wrote…

For better re-usability, I think it's better to return the err to the caller than crashing the program.
Except this, the PR looks good to me.

Good point. Fixed

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 3 files at r1, 2 of 2 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @codexnull and @gitlw)

@codexnull codexnull merged commit fd9411c into master Mar 4, 2019
@codexnull codexnull deleted the javier/smarter_bulk_out branch March 14, 2019 18:44
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
Fixes hypermodeinc#3034. Summary of changes:

* Add --replace_out option to bulk command.

* Require --replace_out option if
  * Output directory was changed from default, and 
  * Output directory exists, and
  * Output directory is not empty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants