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

Backup vendor at init #560

Merged
merged 2 commits into from May 12, 2017

Conversation

Projects
None yet
5 participants
@darkowlzz
Copy link
Collaborator

darkowlzz commented May 11, 2017

  • Creates backup of vendor directory, if exists with content, at init.
  • Adds tests for BackupVendor() function.
  • Adds RemoveFile() function to test helper.

Fixes #552

@googlebot googlebot added the cla: yes label May 11, 2017

@darkowlzz darkowlzz force-pushed the darkowlzz:552-backup-vendor-at-init branch from adf0968 to 58a8dc5 May 11, 2017

@sdboyer
Copy link
Member

sdboyer left a comment

thanks for jumping on this! couple things

@@ -617,3 +617,10 @@ func (h *Helper) GetCommit(repo string) string {
}
return strings.TrimSpace(string(out))
}

func (h *Helper) RemoveFile(path string) {
err := os.RemoveAll(path)

This comment has been minimized.

@sdboyer

sdboyer May 12, 2017

Member

If this is really just for removing a single file, then it should probably be reduced to just use os.Remove()

This comment has been minimized.

@darkowlzz

darkowlzz May 12, 2017

Collaborator

No, it's for removing everything, file, directories with files in them. In this PR, it's for deleting a directory. Maybe RemoveDir() would be a better name for the function.

This comment has been minimized.

@sdboyer

sdboyer May 12, 2017

Member

Ah yeah, I see where it's used.

Y'know, let's actually not have this helper, for the same reason we avoid similar ones elsewhere. Putting the error into a helper func obscures the actual spot where it was called from (even though there's only one place right now), which makes it harder to debug tests.

Let's just call os.RemoveAll() directly in the test, and if an error occurs, then t.Fatal(err) is sufficient.

project.go Outdated
// Check if a directory with same name exists
if _, err = os.Stat(vendorbak); os.IsNotExist(err) {
// Copy existing vendor to vendor-{suffix}
if err := CopyDir(vpath, vendorbak); err != nil {

This comment has been minimized.

@sdboyer

sdboyer May 12, 2017

Member

Any reason to use copy here instead of renameWithFallback()?

This comment has been minimized.

@darkowlzz

darkowlzz May 12, 2017

Collaborator

@sdboyer now that I think of it again, renaming makes more sense 😅 no point copying and leaving the file when it would be overwritten in the next phase of init. Will change it.

@@ -617,3 +617,10 @@ func (h *Helper) GetCommit(repo string) string {
}
return strings.TrimSpace(string(out))
}

func (h *Helper) RemoveFile(path string) {
err := os.RemoveAll(path)

This comment has been minimized.

@sdboyer

sdboyer May 12, 2017

Member

Ah yeah, I see where it's used.

Y'know, let's actually not have this helper, for the same reason we avoid similar ones elsewhere. Putting the error into a helper func obscures the actual spot where it was called from (even though there's only one place right now), which makes it harder to debug tests.

Let's just call os.RemoveAll() directly in the test, and if an error occurs, then t.Fatal(err) is sufficient.

darkowlzz added some commits May 11, 2017

Backup vendor at init
- Creates backup of vendor directory, if exists with content, at init.
- Adds tests for BackupVendor() function.
- Adds RemoveFile() function to test helper.
Rename existing vendor instead of copying
- Renames existing vendor instead of copying.
- Remove removeFile() from test helpers. No longer required.

@darkowlzz darkowlzz force-pushed the darkowlzz:552-backup-vendor-at-init branch from 58a8dc5 to 47302cf May 12, 2017

@darkowlzz

This comment has been minimized.

Copy link
Collaborator

darkowlzz commented May 12, 2017

@googlebot ping!
asleep?

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented May 12, 2017

OK, I think this is good. Thanks! 🎉 🎉

@sdboyer sdboyer merged commit 109db8e into golang:master May 12, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate no new or fixed issues
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
return "", err
}
return vendorbak, nil
} else {

This comment has been minimized.

@ericmdantas

This comment has been minimized.

@darkowlzz

darkowlzz May 12, 2017

Collaborator

I think we should abort if we can't backup the existing vendor. Hence returning an error.

This comment has been minimized.

@ericmdantas

ericmdantas May 12, 2017

Sorry, that's not what I meant.

I meant removing the else part and still have the return "", errVendorBackupFailed.

This comment has been minimized.

@darkowlzz

darkowlzz May 13, 2017

Collaborator

yeah, that's right 😅

This comment has been minimized.

@sdboyer

sdboyer May 15, 2017

Member

seems we need the world's tiniest follow-up PR 😄

This comment has been minimized.

@carolynvs

carolynvs May 15, 2017

Collaborator

@sdboyer I think this is it? #578

This comment has been minimized.

@sdboyer

sdboyer May 15, 2017

Member

lol whoops, totally is. slipped off my notifications list, and i hadn't checked the actual PR queue 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment