I think it's the time for someone to verify what I managed to come up with at: https://github.com/erdi/gradle/commit/630003850f9dc562ce7a757ef04ffd5957628a62. Would be cool to know what should be improved and decide what's next - do we merge it in or do we continue with the next story.
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 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 > > > >
