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

Fix failed to symlink overlay files if it already exists #322

Closed
wants to merge 3 commits into from
Closed

Fix failed to symlink overlay files if it already exists #322

wants to merge 3 commits into from

Conversation

starzation
Copy link

@starzation starzation commented Dec 30, 2023

Fixed #321

@apprehensions
Copy link
Member

Please take this to the upstream library github.com/otiai10/copy .

@starzation
Copy link
Author

Please take this to the upstream library github.com/otiai10/copy .

Oh ok

@starzation
Copy link
Author

@apprehensions Do you want me to close this pr or merge this while i move this to upstream lib?

@apprehensions
Copy link
Member

Discuss with library maintainer to determine if this patch belongs there or here.

@starzation starzation changed the title Fix symlink error file exists (#321) Fix failed to symlink overlay files if it already exists Dec 30, 2023
@starzation starzation marked this pull request as ready for review January 12, 2024 14:41
@starzation
Copy link
Author

starzation commented Jan 12, 2024

@apprehensions Found out it wasn't upstream lib issue we need to check if file already exists first. (otiai10/copy#132)

@apprehensions
Copy link
Member

Sorry for getting to this PR late, but does the solution have to be this complicated?

@apprehensions
Copy link
Member

If possible, it would be greatly appreciate to test this, just to enforce and proof the new behavior. A guide on how to make Go tests can be found here.

@starzation
Copy link
Author

If possible, it would be greatly appreciate to test this, just to enforce and proof the new behavior. A guide on how to make Go tests can be found here.

I have added test cases, I am not sure if this is what you want.

@apprehensions
Copy link
Member

apprehensions commented Feb 6, 2024

Please fix conflicts for merge. Thanks for your work as well.

Do try to use slog as well for logging.

@starzation
Copy link
Author

starzation commented Feb 6, 2024

Please fix conflicts for merge. Thanks for your work as well.

Do try to use slog as well for logging.

Done!

internal/dirs/copy_test.go Outdated Show resolved Hide resolved
internal/dirs/copy.go Outdated Show resolved Hide resolved
internal/dirs/copy.go Outdated Show resolved Hide resolved
@astraness
Copy link

@apprehensions Maybe you can close this now since otiai10/copy#145 has been merged.

@apprehensions
Copy link
Member

I have asked the pull request and the issue author to verify if the issue has been solved. If it is for you having faced this issue, please let me know.
To do that a Go module upgrade will be required.

@apprehensions
Copy link
Member

I have checked myself and the PR fixes it.

@apprehensions
Copy link
Member

Thank you for your work @starzation

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.

Failed to symlink overlay files if it already exists
3 participants