launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06860
[Merge] lp:~allenap/maas/anon-get-file-follow-up into lp:maas
Gavin Panella has proposed merging lp:~allenap/maas/anon-get-file-follow-up into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/maas/anon-get-file-follow-up/+merge/98919
This addresses my review comments in:
https://code.launchpad.net/~julian-edwards/maas/anon-get-file/+merge/98798
> [1]
>
> +def get_file(request):
> + filename = request.GET.get("filename", None)
>
> Could do with a docstring, though I guess it's fairly
> self-explanatory.
>
> [2]
>
> +@api_operations
> +class AnonFilesHandler(AnonymousBaseHandler):
> + """Anonymous file operations."""
> + allowed_methods = ('GET',)
>
> This really needs explanation as to why it's a sensible thing to
> do. From talking with Francis and Kapil, it seems that (a) file
> names will be uuid-like or contain some unguessable nonce, and (b)
> this is required so that all the agents within the environment can
> see the file without needing provider credentials.
>
> [3]
>
> + def setUp(self):
> ...
> + os.mkdir(self.tmpdir)
>
> This is repeated twice, and ought to be extracted.
It also fixes some lint.
--
https://code.launchpad.net/~allenap/maas/anon-get-file-follow-up/+merge/98919
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/maas/anon-get-file-follow-up into lp:maas.
=== modified file 'Makefile'
--- Makefile 2012-03-19 16:44:14 +0000
+++ Makefile 2012-03-22 20:30:25 +0000
@@ -52,9 +52,10 @@
bin/test.maas
bin/test.pserv
-lint: sources = setup.py src templates utilities
+lint: sources = contrib setup.py src templates twisted utilities
lint: bin/flake8
- @bin/flake8 $(sources)
+ @find $(sources) -name '*.py' ! -path '*/migrations/*' \
+ -print0 | xargs -r0 bin/flake8
check: clean test
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-03-22 15:30:45 +0000
+++ src/maasserver/api.py 2012-03-22 20:30:25 +0000
@@ -502,7 +502,13 @@
return ('node_mac_handler', [node_system_id, mac_address])
-def get_file(request):
+def get_file(handler, request):
+ """Get a named file from the file storage.
+
+ :param filename: The exact name of the file you want to get.
+ :type filename: string
+ :return: The file is returned in the response content.
+ """
filename = request.GET.get("filename", None)
if not filename:
raise MAASAPIBadRequest("Filename not supplied")
@@ -515,18 +521,20 @@
@api_operations
class AnonFilesHandler(AnonymousBaseHandler):
- """Anonymous file operations."""
+ """Anonymous file operations.
+
+ This is needed for Juju. The story goes something like this:
+
+ - The Juju provider will upload a file using an "unguessable" name.
+
+ - The name of this file (or its URL) will be shared with all the agents in
+ the environment. They cannot modify the file, but they can access it
+ without credentials.
+
+ """
allowed_methods = ('GET',)
- @api_exported('get', 'GET')
- def get(self, request):
- """Get a named file from the file storage.
-
- :param filename: The exact name of the file you want to get.
- :type filename: string
- :return: The file is returned in the response content.
- """
- return get_file(request)
+ get = api_exported('get', 'GET')(get_file)
@api_operations
@@ -535,15 +543,7 @@
allowed_methods = ('GET', 'POST',)
anonymous = AnonFilesHandler
- @api_exported('get', 'GET')
- def get(self, request):
- """Get a named file from the file storage.
-
- :param filename: The exact name of the file you want to get.
- :type filename: string
- :return: The file is returned in the response content.
- """
- return get_file(request)
+ get = api_exported('get', 'GET')(get_file)
@api_exported('add', 'POST')
def add(self, request):
=== modified file 'src/maasserver/migrations/0002_add_token_to_node.py'
--- src/maasserver/migrations/0002_add_token_to_node.py 2012-03-21 12:02:11 +0000
+++ src/maasserver/migrations/0002_add_token_to_node.py 2012-03-22 20:30:25 +0000
@@ -4,9 +4,11 @@
# encoding: utf-8
import datetime
+
+from django.db import models
from south.db import db
from south.v2 import SchemaMigration
-from django.db import models
+
class Migration(SchemaMigration):
=== modified file 'src/maasserver/migrations/0002_macaddress_unique.py'
--- src/maasserver/migrations/0002_macaddress_unique.py 2012-03-22 11:56:17 +0000
+++ src/maasserver/migrations/0002_macaddress_unique.py 2012-03-22 20:30:25 +0000
@@ -1,8 +1,10 @@
# encoding: utf-8
import datetime
+
+from django.db import models
from south.db import db
from south.v2 import SchemaMigration
-from django.db import models
+
class Migration(SchemaMigration):
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-03-22 17:10:40 +0000
+++ src/maasserver/tests/test_api.py 2012-03-22 20:30:25 +0000
@@ -19,6 +19,7 @@
from django.conf import settings
from django.db.models.signals import post_save
+from fixtures import Fixture
from maasserver.api import extract_oauth_key
from maasserver.models import (
ARCHITECTURE_CHOICES,
@@ -212,8 +213,12 @@
mac = 'aa:bb:cc:dd:ee:ff'
factory.make_mac_address(mac)
architecture = factory.getRandomChoice(ARCHITECTURE_CHOICES)
+
def node_created(sender, instance, created, **kwargs):
- self.assertFalse(True)
+ self.fail(
+ "The post_save event for %r should not have "
+ "been fired." % instance)
+
post_save.connect(node_created, sender=Node)
self.client.post(
self.get_uri('nodes/'),
@@ -296,8 +301,12 @@
self.assertEqual(httplib.FORBIDDEN, response.status_code)
+class AnonAPITestCase(APIv10TestMixin, TestCase):
+ """Base class for anonymous API tests."""
+
+
class APITestCase(APIv10TestMixin, TestCase):
- """Extension to `TestCase`: log in first.
+ """Base class for logged-in API tests.
:ivar logged_in_user: A user who is currently logged in and can access
the API.
@@ -707,7 +716,7 @@
token=second_token)
user_2 = factory.make_user()
- user_2_token = create_auth_token(user_2)
+ create_auth_token(user_2)
factory.make_node(
owner=self.logged_in_user, status=NODE_STATUS.ALLOCATED,
token=second_token)
@@ -887,7 +896,7 @@
def test_macs_DELETE_not_found(self):
# When deleting a MAC Address, the api returns a 'Not Found' (404)
# error if no existing MAC Address is found.
- node = factory.make_node()
+ node = factory.make_node()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
node.system_id, '00-aa-22-cc-44-dd'))
@@ -897,7 +906,7 @@
def test_macs_DELETE_bad_request(self):
# When deleting a MAC Address, the api returns a 'Bad Request' (400)
# error if the provided MAC Address is not valid.
- node = factory.make_node()
+ node = factory.make_node()
response = self.client.delete(
self.get_uri('nodes/%s/macs/%s/') % (
node.system_id, 'invalid-mac'))
@@ -940,7 +949,28 @@
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
-class FileStorageTestMixin:
+class MediaRootFixture(Fixture):
+ """Create and clear-down a `settings.MEDIA_ROOT` directory.
+
+ The directory must not previously exist.
+ """
+
+ def setUp(self):
+ super(MediaRootFixture, self).setUp()
+ self.path = settings.MEDIA_ROOT
+ if os.path.exists(self.path):
+ raise AssertionError("See media/README")
+ self.addCleanup(shutil.rmtree, self.path, ignore_errors=True)
+ os.mkdir(self.path)
+
+
+class FileStorageAPITestMixin:
+
+ def setUp(self):
+ super(FileStorageAPITestMixin, self).setUp()
+ media_root = self.useFixture(MediaRootFixture()).path
+ self.tmpdir = os.path.join(media_root, "testing")
+ os.mkdir(self.tmpdir)
def make_file(self, name="foo", contents="test file contents"):
"""Make a temp file named `name` with contents `contents`.
@@ -973,17 +1003,7 @@
return self.client.get(self.get_uri('files/'), params)
-class AnonymousFileStorageAPITest(APIv10TestMixin, FileStorageTestMixin,
- TestCase):
-
- def setUp(self):
- super(AnonymousFileStorageAPITest, self).setUp()
- media_root = settings.MEDIA_ROOT
- self.assertFalse(os.path.exists(media_root), "See media/README")
- self.addCleanup(shutil.rmtree, media_root, ignore_errors=True)
- os.mkdir(media_root)
- self.tmpdir = os.path.join(media_root, "testing")
- os.mkdir(self.tmpdir)
+class AnonymousFileStorageAPITest(FileStorageAPITestMixin, AnonAPITestCase):
def test_get_works_anonymously(self):
factory.make_file_storage(filename="foofilers", data=b"give me rope")
@@ -993,16 +1013,7 @@
self.assertEqual(b"give me rope", response.content)
-class FileStorageAPITest(APITestCase, FileStorageTestMixin):
-
- def setUp(self):
- super(FileStorageAPITest, self).setUp()
- media_root = settings.MEDIA_ROOT
- self.assertFalse(os.path.exists(media_root), "See media/README")
- self.addCleanup(shutil.rmtree, media_root, ignore_errors=True)
- os.mkdir(media_root)
- self.tmpdir = os.path.join(media_root, "testing")
- os.mkdir(self.tmpdir)
+class FileStorageAPITest(FileStorageAPITestMixin, APITestCase):
def test_add_file_succeeds(self):
filepath = self.make_file()
=== modified file 'src/maasserver/urls.py'
--- src/maasserver/urls.py 2012-03-21 18:53:44 +0000
+++ src/maasserver/urls.py 2012-03-22 20:30:25 +0000
@@ -35,8 +35,8 @@
KeystoreView,
login,
logout,
+ NodeEdit,
NodeListView,
- NodeEdit,
NodesCreateView,
NodeView,
proxy_to_longpoll,
=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py 2012-03-22 15:10:57 +0000
+++ src/maasserver/views.py 2012-03-22 20:30:25 +0000
@@ -25,7 +25,6 @@
parse_qs,
)
from django.conf import settings as django_settings
-from django.core.exceptions import PermissionDenied
from django.contrib import messages
from django.contrib.auth.forms import PasswordChangeForm as PasswordForm
from django.contrib.auth.models import User
@@ -33,6 +32,7 @@
login as dj_login,
logout as dj_logout,
)
+from django.core.exceptions import PermissionDenied
from django.core.urlresolvers import reverse
from django.http import (
HttpResponse,
@@ -61,8 +61,8 @@
NewUserCreationForm,
ProfileForm,
UbuntuForm,
+ UIAdminNodeEditForm,
UINodeEditForm,
- UIAdminNodeEditForm,
)
from maasserver.messages import messaging
from maasserver.models import (
=== modified file 'templates/script.py'
--- templates/script.py 2012-01-24 12:44:48 +0000
+++ templates/script.py 2012-03-22 20:30:25 +0000
@@ -13,7 +13,6 @@
import argparse
-
# See http://docs.python.org/release/2.7/library/argparse.html.
argument_parser = argparse.ArgumentParser(description=__doc__)
