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

Add optional interfaces for Symlink and Readlink #228

Merged
merged 4 commits into from
May 20, 2020
Merged

Add optional interfaces for Symlink and Readlink #228

merged 4 commits into from
May 20, 2020

Conversation

Maldris
Copy link
Contributor

@Maldris Maldris commented Dec 11, 2019

I created two interfaces to facilitate file systems optionally supporting the Symlink and Readlink operations from the os implementation.

type Linker interface {
	SymlinkIfPossible(oldname, newname string) error
}
type LinkReader interface {
	ReadlinkIfPossible(name string) (string, error)
}

I opted to write them this way to preserve consistency with the Lstater interface created by @bep

The actual Symlink and Readlink are only implemented in the OsFs, with proxy support in:

  • BasePathFs,
  • CopyOnWriteFs, and
  • ReadOnlyFs

for consistency with the other two I implemented the Linker interface on ReadOnlyFs with it just returning a not supported error. I would appreciate feedback if this is the correct pattern, or if I should instead remove the method?

The motivation for this change was to allow wrapping an existing use of afero in a project with the go-billy interface to allow a git integration to support a client request.
The implementation is in the process of being tested in our logic, but I wanted to create the pull request and get feedback and make any necessary changes early.

@Maldris
Copy link
Contributor Author

Maldris commented May 18, 2020

ok, got sidetracked with some family things and then assigned to a different project, but I've had a chance to come back to this.

I made use of this branch to make a wrapper for compatibility with https://github.com/src-d/go-git.
This implementation can be found at https://github.com/Maldris/go-billy-afero.

Go-git is used in an application we have which otherwise uses afero for other operations. After writing the wrapper we wrote a full test that sequestially tested the core git operations one after another, including its use of symlink and readLink.

other than some issues with the tests themselves, this appears to work without issue on windows, ubuntu and an alpine based docker container (I don't have access to a darwin system to test).

I do have some stylistic questions prior to requesting a final merge howerver:
For consistency with the os package's behaviour of Symlink in which the returned error is always a os.LinkError. I've returned a os.LinkError wrapping the "symlink not supported" error. Should I instead be returning the base error so it may be tested against, instead of requiring the user to unwrap the error? or is constancy with the os package behaviour better in this case?

@0xmichalis
Copy link
Collaborator

@Maldris consistency with the os package is nice to have.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@Maldris
Copy link
Contributor Author

Maldris commented May 20, 2020

In that case, are there any changes (functional or stylistic) you would advise before this can be merged?

If not, I'll rebase the branch against master and it should be good to go.

@0xmichalis
Copy link
Collaborator

@Maldris your changes lgtm, please rebase and I'll merge.

@Maldris
Copy link
Contributor Author

Maldris commented May 20, 2020

ok, clean rebase pushed, and the builds still pass.
Thank you for your time and assistance.

@0xmichalis 0xmichalis merged commit a7dc6ae into spf13:master May 20, 2020
@LandonTClipp
Copy link
Contributor

Can we get a release for this? I would really love to use this feature <3

@LandonTClipp
Copy link
Contributor

@Kargakis

@0xmichalis
Copy link
Collaborator

v1.3.0 is now live

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

4 participants