On 20 Mar 2014, at 6:42 am, Marcin Erdmann <marcin.erdm...@proxerd.pl> wrote:
> 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. 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. > > 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. > 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
