On Wed, Mar 19, 2014 at 10:55 PM, Adam Murdoch <adam.murd...@gradleware.com>wrote:
> > Your changes look really good to me. I added some comments on the commit, > but I think they are all things we can address in later commits. I'd say > let's merge this and go from there. > I created a pull request (https://github.com/gradle/gradle/pull/256) and will address your comments. > > > Some notes: > - As suggested I didn't implement a test for publishing to a repo with > multiple artifact/ivy patterns. > - I decided not to implement a test for generic error handling when > "server throws an exception" - I couldn't come up with a way to simulate > such situation using current SFTPServer fixture > > > It becomes important for caching, as we want to remember 'not found' > differently to 'it blew up'. > > I suspect you can get an internal failure by throwing an exception out of > the VirtualFileSystemView. > Ok, I will change SFTPServer to use a VirtualFileSystemView from a field so that a different instance can be provided on a per test basis. > > and I've learned that when IOExceptions are thrown from a resource > accessor, uploader or lister then gradle provides decent error messages > about what it was doing when a failure occurred. Also, when an unexpected > response is coming back from the server SSHD's sftp client puts status code > in the exception message it throws so it is possible to learn what exactly > has happened. Maybe not the easiest tasks as it involves looking at the > client code or sftp specs to learn what that given error code means but > it's possible > - I didn't do anything special for permission errors. It would involve > quite a lot of changes to SSHD's sftp client and I didn't think that this > is of such significance to actually put the effort in > - I moved transport instantiation from DefaultIvyRepository to > RepositoryTransport factory. It felt a bit premature is it's still not used > by the maven part of things but I did what the design document said > - I did not implement getResourceSha1() in SftpResourceAccessor - the > contract says that it is allowed to return null and none of the test that > were required exercise that method so it didn't feel right to implement it > at this point in time > - I had to create IvySftpModule even though it only delegtes to a backing > repository and return it from IvySftpRepository.module() to > get IvySftpRepository to compile > - I simplified SFTPServer after VirtualFileSystemView was added in a > recent version of SSHD. I needed to update dependency on SSHD as the > SftpClient was only introduced recently. > > Cheers, > Marcin > > > On Sun, Mar 16, 2014 at 8:04 PM, Adam Murdoch <adam.murd...@gradleware.com > > wrote: > >> >> On 16 Mar 2014, at 12:38 am, Marcin Erdmann <marcin.erdm...@proxerd.pl> >> wrote: >> >> I have another question. One of test cases in the spec is Publish via >> 'sftp' to ivy repository with multiple ivyPattern and artifactPattern >> configured - what do we want to verify here? >> >> >> I wouldn't bother. Provided that we have test coverage that publishing >> with a single pattern works for every transport, then the multiple pattern >> case can be covered with a single transport. >> >> I've noticed that when publishing to such a repository the first ivy >> pattern and artifact pattern are used. Should the test case assert that >> this happens? >> >> >> On Fri, Mar 14, 2014 at 8:22 PM, Marcin Erdmann < >> marcin.erdm...@proxerd.pl> wrote: >> >>> >>> >>> >>> On Fri, Mar 14, 2014 at 7:56 PM, Adam Murdoch < >>> adam.murd...@gradleware.com> wrote: >>> >>>> >>>> Yet, there is plenty to be done on error handling and making sure all >>>> resources are being released even in error scenarios - so the boring but >>>> important parts. >>>> >>>> I have one question about error handling. There are three types of >>>> failures listed in the spec: invalid credentials, cannot connect to the >>>> server and everything else (called server exception). I think that we >>>> probably want to extract all permission related errors (when both resolving >>>> and uploading) as a separate error type so that the error message provides >>>> user an idea that the error is permission related as it feels that it might >>>> be a common error. What do you think? >>>> >>>> >>>> The key is to make the error message informative at this stage, which >>>> we don't necessarily need a new type for. If we formalise 'not authorised' >>>> problems in the resource APIs then we would also need to update the file >>>> and http backed resource implementations to. So, I'd leave this as a later >>>> refactoring. >>>> >>> >>> What I meant here by a new type is not a new exception class but to >>> differentiate permission errors from generic errors when it comes to what >>> message is being shown when such an error occurs. At the moment I'm >>> throwing a new runtime exception of type SftpException with a message >>> saying what has happened (invalid credentials, connection error) and where >>> (host and port part of the url). I was asking if we should check if we have >>> permissions to perform a given operation (file download or upload, >>> directory creation, etc) and if not throw a SftpException with a message >>> saying that permission was denied to do it. >>> >>> >>>> >>>> >>>> Also, DefaultExternalResourceRepository requires an implementation of >>>> an ExternalResourceLister yet it is currently not exercised by any test - >>>> to be honest I haven't looked into how it's used and what kind of tests >>>> would be needed for it so any pointers would be appreciated. Do we want to >>>> add some tests to the initial story for that? >>>> >>>> >>>> It's used whenever there's a dynamic version in a dependency >>>> declaration. So, if you've got one of these in one of the tests somewhere, >>>> then we're good. >>>> >>> >>> I don't have a test like that yet so I will add some. Thanks for >>> pointing me in the right direction. >>> >>> >>>> >>>> >>>> >>>> On Thu, Mar 6, 2014 at 3:56 AM, Adam Murdoch < >>>> adam.murd...@gradleware.com> wrote: >>>> >>>>> >>>>> On 6 Mar 2014, at 7:33 am, Marcin Erdmann <marcin.erdm...@proxerd.pl> >>>>> wrote: >>>>> >>>>> I finally got my head around the contract of ExternalResourceAccessor >>>>> and after doing some reverse engineering of HttpResourceAccessor and >>>>> Ivy's SFTPResolver as well as skimming SFTP protocol specs the following >>>>> are my findings and questions: >>>>> >>>>> Error handling is far from perfect in both clients - they only return >>>>> a generic error when you try to get file metadata for a file that doesn't >>>>> exist but there seems to be a specific substatus for such situation >>>>> called SSH_FX_NO_SUCH_FILE. >>>>> I can see that this substatus is returned by SSHD's implementation of SFTP >>>>> server when accessing files that don't exist even though the spec( >>>>> http://tools.ietf.org/id/draft-ietf-secsh-filexfer-00.txt)cryptically >>>>> describes it as "is >>>>> returned when a reference is made to a file which should exist but >>>>> doesn't" - this should but doesn't part baffles me a bit. Anyway I will >>>>> assume that this substatus is returned whenever trying to stat a >>>>> non-existing file. >>>>> >>>>> I'm going to use SSHD's client as it seems to be better maintained and >>>>> I like it more. I will be also able to easily change it's behavior when it >>>>> comes to handling the aforementioned SSH_FX_NO_SUCH_FILE substatus. >>>>> >>>>> We will probably want to throw a specific contextual exception when >>>>> credentials are incorrect, right? >>>>> >>>>> >>>>> At this stage, I think it's sufficient that the error message gives >>>>> the user some idea that the credentials are incorrect. Eventually, it >>>>> would >>>>> be nice to tell the difference, if possible. >>>>> >>>>> >>>>> It feels like we want to have separate ssh sessions for getting >>>>> metadata and getting the file - the fact that you request an >>>>> ExternalResource >>>>> doesn't mean that you will ever read it and thus close it, but you need to >>>>> open a ssh session to get the metadata. So the idea here is to open a >>>>> session, get metadata to see if the resource exists and close the session >>>>> possibly passing the metadata to the created ExternalResource if the >>>>> resource exists. Then, if resource contents are requested open a new >>>>> session and close it when close() is called on the ExternalResource. Are >>>>> we >>>>> ok with such approach? >>>>> >>>>> >>>>> Yes, for now. I think at some point we will rework >>>>> the ExternalResourceAccessor interface so that it can better deal with the >>>>> fact that for some transports (file, sftp), the meta-data for a resource >>>>> and the content for a resource have to be fetched as separate operations. >>>>> The interface at the moment assumes that it's cheap-ish to get the >>>>> meta-data at the same time the content is requested. >>>>> >>>>> This way, the caller can decide whether it needs the meta-data or not, >>>>> and invoke the appropriate operations - get meta-data, get content or get >>>>> meta-data and content (if supported). >>>>> >>>>> >>>>> -- >>>>> Adam Murdoch >>>>> Gradle Co-founder >>>>> http://www.gradle.org >>>>> VP of Engineering, Gradleware Inc. - Gradle Training, >>>>> Support, Consulting >>>>> http://www.gradleware.com >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Adam Murdoch >>>> Gradle Co-founder >>>> http://www.gradle.org >>>> VP of Engineering, Gradleware Inc. - Gradle Training, >>>> Support, Consulting >>>> http://www.gradleware.com >>>> >>>> >>>> >>>> >>> >> >> >> -- >> Adam Murdoch >> Gradle Co-founder >> http://www.gradle.org >> VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting >> http://www.gradleware.com >> >> >> >> > > > -- > Adam Murdoch > Gradle Co-founder > http://www.gradle.org > VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting > http://www.gradleware.com > > > >
