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

[breaking] copy*(): allow copying broken symlinks #779

Merged
merged 3 commits into from May 16, 2020
Merged

Conversation

manidlou
Copy link
Collaborator

@manidlou manidlou commented Mar 26, 2020

Fixes #765, fixes #638, fixes #761.

Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

Looks good. This would be semver-major, right?

@manidlou
Copy link
Collaborator Author

manidlou commented Apr 1, 2020

This would be semver-major, right?

That's right @RyanZim.

@manidlou
Copy link
Collaborator Author

manidlou commented May 5, 2020

@RyanZim should we wait for more approvals or this is ready to be merged?

Copy link
Collaborator

@JPeer264 JPeer264 left a comment

Choose a reason for hiding this comment

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

Do we have enough test cases where we use the actual default dereference: false? Seems like a lot of tests have changed to true. I'm not sure if these tests with the default false are actually needed

@RyanZim
Copy link
Collaborator

RyanZim commented May 5, 2020

@manidlou I was holding off merging since it's semver-major, and I wasn't sure if I wanted to release a major for just this, or bundle in some other changes.

@manidlou
Copy link
Collaborator Author

manidlou commented May 7, 2020

I was holding off merging since it's semver-major, and I wasn't sure if I wanted to release a major for just this, or bundle in some other changes.

@RyanZim sounds good! So we can add this to the 10.0.0 milestone then.

@manidlou
Copy link
Collaborator Author

manidlou commented May 7, 2020

Do we have enough test cases where we use the actual default dereference: false? Seems like a lot of tests have changed to true. I'm not sure if these tests with the default false are actually needed

@JPeer264 most of those tests that are changed to dereference: true are edge cases that they needed to be changed because of the broken changes (new behavior). For the case dereference: false I added new test in the broken symlink test file. But generally, it doesn't hurt do have more tests for the case dereference: false!

@manidlou manidlou changed the title copy*(): allow copying broken symlinks [breaking] copy*(): allow copying broken symlinks May 7, 2020
@manidlou
Copy link
Collaborator Author

manidlou commented May 7, 2020

@RyanZim I found a fix for #759 that will have a merge conflict with this later on. So I am gonna create v10 branch and we will merge this to v10 branch. That way I can move forward with the #759 fix.

Feel free to let me know if you have any objections!

@manidlou
Copy link
Collaborator Author

manidlou commented May 8, 2020

@RyanZim @JPeer264 what is the deal with this branch ci/travis-osximage?! Do we still need to keep it?

@JPeer264
Copy link
Collaborator

JPeer264 commented May 8, 2020

@manidlou thanks for mentioning that. I removed the branch, as it was no longer in use.

@manidlou manidlou added this to the 10.0.0 milestone May 9, 2020
@manidlou manidlou changed the base branch from master to v10 May 16, 2020 04:03
@manidlou manidlou merged commit 403060c into v10 May 16, 2020
@manidlou manidlou deleted the mani/copy-err-msg branch May 16, 2020 04:25
@RyanZim
Copy link
Collaborator

RyanZim commented Jul 29, 2020

@manidlou Just realized this was merged with a merge-commit into v10; do you mind if I recreate the branch with a squash-merge?

@manidlou
Copy link
Collaborator Author

@RyanZim go for it.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 1, 2021

Major version branches are a pain in general, I've decided to just commit to it, and start landing semver-major stuff on master; landed in f21048b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants