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

feat(fs): API for replacing os calls #14280

Merged
merged 14 commits into from
Aug 7, 2019
Merged

feat(fs): API for replacing os calls #14280

merged 14 commits into from
Aug 7, 2019

Conversation

maxunt
Copy link
Contributor

@maxunt maxunt commented Jul 8, 2019

This PR replaces calls to os.Create and os.Rename with our own API for consistent error messaging and to prevent writing over files that already exist

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Documentation updated or issue created (provide link to issue/pr)
  • Signed CLA (if not already signed)

Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

I don't think you should have updated the Python code? Other than that this looks great!

@@ -697,15 +698,15 @@ def package(build_output, pkg_name, version, nightly=False, iteration=1, static=
# logging.debug("Changing package output version from {} to {} for RPM.".format(version, package_version))
# Strip nightly version from package name
new_outfile = outfile.replace("{}-{}".format(package_version, package_iteration), "nightly")
os.rename(outfile, new_outfile)
fs.RenameFile(outfile, new_outfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's going on with this Python code?

@e-dard e-dard requested a review from a team July 23, 2019 13:10
@ghost ghost requested review from benbjohnson and tmgordeeva and removed request for a team July 23, 2019 13:10
@e-dard
Copy link
Contributor

e-dard commented Jul 23, 2019

@maxunt needs at least one more careful review.

panic(err)
}
return name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit but it's not clear from the name that it's writing its own name to the filename. I think it would be clearer from the caller to pass in a string for the data to write. Also, probably worth adding an error check on os.File.WriteString():

// MustCreateTempFile creates a temporary file returning the path to the file.
func MustCreateTempFile(data string) string {
	f, err := ioutil.TempFile("", "fs-test")
	if err != nil {
		panic(fmt.Sprintf("failed to create temp file: %v", err))
	} else if _, err := f.WriteString(data); err != nil {
		panic(err)
	} else if err := f.Close(); err != nil {
		panic(err)
	}
	return name
}

Another approach I've been doing more lately is making the function a helper and naming it something like CreateTempFileOrFail(). That way it only fails the test instead of the whole program execution (although temp files are unlikely to fail).

// CreateTempFileOrFail creates a temporary file returning the path to the file.
func MustCreateTempFile(t testing.TB, data string) string {
	tb.Helper()
	
	f, err := ioutil.TempFile("", "fs-test")
	if err != nil {
		tb.Fatalf("failed to create temp file: %v", err)
	} else if _, err := f.WriteString(data); err != nil {
		t.Fatal(err)
	} else if err := f.Close(); err != nil {
		t.Fatal(err)
	}
	return name
}

Copy link

@tmgordeeva tmgordeeva left a comment

Choose a reason for hiding this comment

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

LGTM, had a small question.

// of oldpath. If this function returns successfully, the contents of newpath will
// be identical to oldpath, and oldpath will be removed.
func RenameFileWithReplacement(oldpath, newpath string) error {
if _, err := os.Stat(newpath); err == nil {

Choose a reason for hiding this comment

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

Would it be safer to rename the existing newpath to a temporary file before renaming oldpath, and then doing the removal last?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure what you are asking, but using RenameFileWithReplacement is safe if you expect and are planning for any old contents of newpath to be truncated and lost. The RenameFile func exists as a tool to ensure you do not lose any data accidentally.

@maxunt maxunt changed the title fs API for replacing os calls feat(fs): API for replacing os calls Jul 25, 2019
@maxunt maxunt merged commit 757fb4f into master Aug 7, 2019
@jacobmarble jacobmarble deleted the er-rename branch October 14, 2019 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants