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

smaller set of changes #608

Merged
merged 4 commits into from
Sep 27, 2016
Merged

smaller set of changes #608

merged 4 commits into from
Sep 27, 2016

Conversation

ineiti
Copy link
Member

@ineiti ineiti commented Sep 23, 2016

  • Copy for copying files

@ineiti ineiti self-assigned this Sep 23, 2016
@ineiti
Copy link
Member Author

ineiti commented Sep 26, 2016

adding test for copy so that coverall is happy...

@@ -288,3 +289,27 @@ func InputYN(def bool, args ...interface{}) bool {
}
return strings.ToLower(string(Input(defStr, args...)[0])) == "y"
}

// Copy makes a copy of a local file with the same file-mode-bits set.
func Copy(dst, src string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there is not another place where you put it in one of the PRs coming from the nsdi code ? Just asking if this is implemented twice or not, because I saw this piece of code in different PRs so I wonder...

Copy link
Member Author

Choose a reason for hiding this comment

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

I just searched, and it's neither in master, nor in nsdi_skipchain_602.

@liamsi
Copy link
Member

liamsi commented Sep 26, 2016

adding test for copy so that coverall is happy...

Although it is nice to add a test maybe we should configure coveralls not to fail if the coverage decreases?

@liamsi liamsi closed this Sep 26, 2016
@liamsi liamsi reopened this Sep 26, 2016
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

This seem to be changes I did on the first timestamp branch. I think it's totally fine to loose some of the history but I don't think this test isn't really neccessary (I forgot to delete it)

@nikkolasg
Copy link
Contributor

Although it is nice to add a test maybe we should configure coveralls not to fail if the coverage decreases?

NOPE ! Please let's try to have high standards on master when we can. If really we need to merge something with a decreasing coverage, we can still do it anyway and we'll have a warning as a bonus.

@liamsi
Copy link
Member

liamsi commented Sep 26, 2016

OK, fair enough. Coveralls still complains about the coverage decreasing.

Copy link
Contributor

@nikkolasg nikkolasg left a comment

Choose a reason for hiding this comment

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

ok for me

@liamsi liamsi merged commit 36c79a3 into master Sep 27, 2016
@liamsi liamsi deleted the nsdi_changes_602 branch September 27, 2016 08:46
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.

None yet

3 participants