Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/416109-arbitrary-endpoints into lp:txaws
On Sun, 2009-08-23 at 21:30 +, Robert Collins wrote: === modified file 'txaws/client/gui/gtk.py' --- txaws/client/gui/gtk.py 2009-08-18 22:53:53 + +++ txaws/client/gui/gtk.py 2009-08-20 19:14:32 + @@ -8,7 +8,7 @@ import gobject import gtk -from txaws.credentials import AWSCredentials +from txaws.ec2.service import EC2Service __all__ = ['main'] @@ -27,10 +27,10 @@ # Nested import because otherwise we get 'reactor already installed'. self.password_dialog = None try: -creds = AWSCredentials() +service = AWSService() This is going to be a NameError :P ... === added file 'txaws/credentials.py' this file already exists. I think you've done something weird in your branch. lets review once thats fixed. Find me in #bzr :P review needsfixing -- https://code.launchpad.net/~oubiwann/txaws/416109-arbitrary-endpoints/+merge/10581 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/416109-arbitrary-endpoints into lp:txaws
On Thu, 2009-08-20 at 19:25 +, Duncan McGreggor wrote: Duncan McGreggor has proposed merging lp:~oubiwann/txaws/416109-arbitrary-endpoints into lp:txaws. Requested reviews: txAWS Team (txawsteam) This branch adds support for a service object that manages host endpoints as well as authorization keys (thus obviating the need for the AWSCredential object). Lets be careful to keep space under storage, ec2 etc for server components. storage.service isn't really a storage service :) Lets call the description of an end point AWSServiceEndpoint, or something like that. local and credentials appear orthogonal to me - for instance, EC2 EU and EC2 US are different endpoints/services with common credentials. I think conflating them is unnecessary and undesirable. Further to that, the AWSCredentials are usable on related services in a single region - EC2, S3 and so on, so when we're passing around a description, we probably want to have a region that describes the endpoints for a collection of services. The goal being able to have a static object AWS_US1 = #... AWS_US2 = #... and for people to make their own; my_eucalyptus_region = #... At runtime then, one would ask a region for a client of a particular service, using some credentials. AWS_US1.make_ec2_client(my_creds) AWS_US1.make_sqs_client(my_creds) etc. We could do this without changing the existing clients at all, by just storing scheme,host tuples in a AWSRegion - but I think it is cleaner to do the sort of refactoring you have done. I think it would be best by having an AWSServiceEndpoint which has the scheme and url, and keeping the creds separate. For instance, class AWSServiceRegion: def make_ec2_client(self, creds=None): return EC2Client(creds=creds, service_endpoint=self.ec2_endpoint) Also a bit of detail review - 'default_schema = https' - in URL terms (see http://www.ietf.org/rfc/rfc3986.txt) that is a _scheme_, not a _schema_. review needsfixing -- https://code.launchpad.net/~oubiwann/txaws/416109-arbitrary-endpoints/+merge/10477 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/415491-instance-object into lp:txaws
+1 -- https://code.launchpad.net/~oubiwann/txaws/415491-instance-object/+merge/10505 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~statik/txaws/here-have-some-s4 into lp:txaws
Wow, code explosion. Uhm, This perhaps would be easier to review in parts... Firstly, the module should be txaws.storage.simulator, I think. Or storage.server.simulator. Secondly, copyright headers. txaws does a much leaner one; # Copyright (C) 2009 $PUT_YOUR_NAMES_HERE # Licenced under the txaws licence available at /LICENSE in the txaws source. Please use that - it prevents skew between different modules. See txaws/ec2/client.py, for instance. Thirdly, I'm not sure what python versions we're aiming for. I note that the simulator code depends on 'with', which carries an implicit version requirement. Please document the version that the simulator supports both in /README and perhaps the docstring for the simulator. On Wed, 2009-08-19 at 14:45 +, Elliot Murphy wrote: === modified file 'README' --- README 2009-04-26 08:32:36 + +++ README 2009-08-19 14:36:56 + @@ -7,6 +7,10 @@ * The epsilon python package (python-epsilon on Debian or similar systems) +* The S4 test server has a dependency on boto (python-boto) on Debian or similar) + This dependency should go away in favor of using txaws infrastructure (s4 was + originally developed separately from txaws) This is a problem :). I'd much rather see code land without having a boto dependency at any point: boto is rather ugly, and the code will likely be a lot nicer right from the get-go if we don't have it. === added file 'txaws/s4/README' see above - the location doesn't fit with the txaws source layout. This is a storage server module (contrast with txaws.storage.client). Having a README in a python module is odd. The content would be better put in the __init__.py's docstring, so that pydoctor etc can show it. --- txaws/s4/README 1970-01-01 00:00:00 + +++ txaws/s4/README 2009-08-19 14:36:56 + @@ -0,0 +1,30 @@ +S4 - a S3 storage system stub += + +the server comes with some sample scripts so you can see how to use it. + +Using twistd + + +to start: ./start-s4.sh +to stop: ./stop-s4.sh + +the sample S4.tac defaults to port 8080. if you want to change that you can create your own S4.tac. Given that this is a twisted process, it would be nice for the docs to say 'to start: twistd -FOO -BAR -BAZ' rather than referring to a shell script which by its nature won't work on windows. === added directory 'txaws/s4/contrib' === added file 'txaws/s4/contrib/S3.py' --- txaws/s4/contrib/S3.py 1970-01-01 00:00:00 + +++ txaws/s4/contrib/S3.py 2009-08-19 14:36:56 + What is this file for? how is it used? It looks like a lot of duplication with existing code in txaws. The different (C) terms means it will need to be mentioned in some way at the top level portion. I suspect we need to add an AUTHORS file too. === added file 'txaws/s4/s4.py' ...+if __name__ == __main__: +root = Root() +site = server.Site(root) +reactor.listenTCP(8808, site) +reactor.run() I've skipped most of this file pending the boto dependency being removed. But I thought I'd mention that this fragment above is highly redundant with shipping a tac - it shouldn't be needed at all. === added file 'txaws/s4/s4_xml.py' --- txaws/s4/s4_xml.py 1970-01-01 00:00:00 + +++ txaws/s4/s4_xml.py 2009-08-19 14:36:56 + Again, I've largely skipped this file - the review is huge and I don't want to make you wait to start getting it ready for me to have time to read the entire thing. embedding a comment like this in the code makes the code harder to read. Putting it in some actual documentation somewhere would be better (or even just reference the AWS spec). ... + +if __name__ == '__main__': +# pylint: disable-msg=W0403 +# pylint: disable-msg=E0611 +from s4 import Bucket +bucket = Bucket(test-bucket) +lbr = ListBucketResult(bucket) +print to_XML(lbr) +print I really don't like this style of adhoc testing - please write a unit test and have it perform a string equality, if thats needed. Otherwise its dead code - it won't be checked regularly, can skew and thats a problem. Or if its not a problem, we don't need it at all :). == added directory 'txaws/s4/testing' Calling this 'test_support' would make it clearer that its not tests. === added directory 'txaws/s4/tests' ... +@defer_to_thread +def test_get(self): ... This is a bit of a smell - why do we need to use threads in testing this code? ... +def test_suite(): +Used by the rest runner to find the tests in this module +return unittest.TestLoader().loadTestsFromName(__name__) + +if __name__ == __main__: +unittest.main() This boilerplate isn't needed: python -m unittest module is precisely equivalent to that. +if __name__ == '__main__': +suite = unittest.TestSuite() +suite.addTest(unittest.makeSuite(S3ConnectionTest)) +unittest.TextTestRunner(verbosity=2).run(suite)
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
On Thu, 2009-08-20 at 02:06 +, Duncan McGreggor wrote: On Wed, Aug 19, 2009 at 10:46 PM, Robert Collinsrobe...@robertcollins.net wrote: Uhm, I'm not sure :) The only remaining thing was import sorting. I don't think you replied to my reply on that. Hrm, well I don't know what you're sorting one... can you explain it? the module name, and the item. from foo import bar from gam import alpha, beta from gam.quux import zulu is well sorted. Why does sorting matter? It reduces merge conflicts - a lot. Because when people add the same import they'll do so at the same line, and different imports will be spread out rather than all at the bottom. -Rob -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~statik/txaws/here-have-some-s4 into lp:txaws
On Thu, 2009-08-20 at 03:36 +, Elliot Murphy wrote: Thanks a lot for the quick review. The code is very much in the state it was being used internally, and I think your comments all make sense and will improve the code. I differ on the license header thing - I explicitly chose not to copy the existing indirect way of specifying the license. You'd need to go back to the copyright holder to change the license anyway, so specifying the license that way is not a good idea IMO. Licensing is important - while we build free/open source software on the hack of using copyright to ensure the right to copy :). I think you'll find all the existing code in txaws uses the pithy approach; the work as a whole is whats licensed : the centralisation isn't to make /changing/ the license easy - its to make auditing, checking and reading code easier. Debian for instance, wants to be sure that all relevant licences are listed; having the licence duplicated in many source files makes that harder. There is also a DRY aspect to it. If you believe there is a risk that the code won't be properly protected (from what - the project licence is MIT - essentially public domain) then we should certainly investigate that further. Otherwise, I don't see what is gained by having the same text duplicated in each file, and really think the shorter reference is much more pleasant. (When I first joined the project, I'm not even sure there _was_ a license :)). Just to set expectations, ...o be up front and explain that this branch will probably sit for a couple of months before lucio or I or Christian will be able to give it that level of attention. I'd actually like to kill the whole contrib directory too. -- That's fine with me too - now its out there its possible for someone to stand up and clean it up too. -Rob -- https://code.launchpad.net/~statik/txaws/here-have-some-s4/+merge/10388 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
