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
