Update patch set 5

Patch Set 5: Code-Review-1 (12 comments) Patch-set: 5 Label: Code-Review=-1
2014-04-29 16:54:22 +00:00
parent 95748d7aba
commit 9c6275ad3c
2 changed files with 209 additions and 0 deletions
--- a/21
+++ b/21
@@ -0,0 +1,21 @@
+{
+  "comments": [
+    {
+      "key": {
+        "uuid": "bad4c104_e188d2c8",
+        "filename": "tests/Tests/Identity/v2/IdentityServicesTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 117,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 0,
+      "message": "Why were these tests removed? With identity v2 you can authenticate without scoping to a token. Why should this case not be tested.",
+      "revId": "2fce47a7f1671e34eae221ca7a086f8baa45e58f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    }
+  ]
+}
--- a/188
+++ b/188
@@ -1,5 +1,175 @@
 {
  "comments": [
+    {
+      "key": {
+        "uuid": "bad4c104_06222cb9",
+        "filename": "phpunit.xml.dist",
+        "patchSetId": 5
+      },
+      "lineNbr": 10,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "Why the indentation change to 8 spaces from 4?",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_a1feca69",
+        "filename": "src/OpenStack/ObjectStore/v1/Resource/StreamWrapper.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 1470,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "This is a carry over comment from patch set 2 that was not responded to...\n\nThe name shouldn\u0027t have swift in it. Instead is should refer to object store. See https://wiki.openstack.org/wiki/SDK-Development\nI do realize other places use the incorrect naming. I crafted https://blueprints.launchpad.net/openstack-sdk-php/+spec/service-name-cleanup to deal with the other cases.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_0cac0ded",
+        "filename": "tests/Tests/Identity/v2/IdentityServicesTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 187,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "If authentication happens without being scoped to a tenant there shouldn\u0027t be a tenant. Why is this check commented out?",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_ec40c937",
+        "filename": "tests/Tests/Identity/v2/IdentityServicesTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 226,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "The name is a configurable option. This should either be removed or made configurable in the settings file.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_4c48d55b",
+        "filename": "tests/Tests/Identity/v2/IdentityServicesTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 235,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "Same as my comment for line 226.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_4ce4553b",
+        "filename": "tests/Tests/ObjectStore/v1/StreamWrapperTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 24,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "ContainerNotEmptyException has been moved to OpenStack\\ObjectStore\\v1\\Exception",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_6c3759b1",
+        "filename": "tests/Tests/ObjectStore/v1/StreamWrapperTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 47,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "Can you remove the added indentation on the comment.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_2c5551c6",
+        "filename": "tests/Tests/ObjectStore/v1/StreamWrapperTest.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 117,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "Why is this tested against the identity tenant while there is a separate swift tenant you\u0027ve added in other parts of the code?",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_214e3ac2",
+        "filename": "tests/Tests/TestCase.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 105,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "This is an addition to the settings but the settings.ini example/dist file is not updated here for it. It\u0027s also not optional here where it was in a previous patch set.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_0110564d",
+        "filename": "tests/bootstrap.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 1,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "What is this file being used for? This was a question Sam asked on a previous patch set. Can you elaborate on what it fixes?\n\nThe second part is that there should be a copyright notice at the top of this file if it has a reason to stay. Based on your previous reviews I think you\u0027ll put the OpenStack Foundation Apache 2 notice.",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
+    },
    {
      "key": {
        "uuid": "bad4c104_3b1d8967",
@@ -70,6 +240,24 @@
      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
      "unresolved": false
+    },
+    {
+      "key": {
+        "uuid": "bad4c104_0193f6a3",
+        "filename": "tests/bootstrap.php",
+        "patchSetId": 5
+      },
+      "lineNbr": 3,
+      "author": {
+        "id": 8512
+      },
+      "writtenOn": "2014-04-29T16:54:22Z",
+      "side": 1,
+      "message": "Terry, We use an external package called composer that is similar to bundler in the Ruby space. To use the packages it managers you need to include the autoloader so it can automatically autoload the packages it manages. That\u0027s what this is.",
+      "parentUuid": "bad4c104_bb6d1907",
+      "revId": "39b4393df0e611e00a2cb2e76c94bfe57918831f",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": false
    }
  ]
 }