eric-maynard commented on code in PR #361:
URL: https://github.com/apache/polaris/pull/361#discussion_r1793835495


##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
       assertThat(response).returns(204, Response::getStatus);
     }
   }
+
+  @Test
+  public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+    // Create a PrincipalRole and a new catalog. Grant the catalog_admin role 
to the new principal
+    // role
+    String principalRoleName = "mypr33";
+    PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+    createPrincipalRole(principalRole1);
+
+    String catalogName = "myuniquetestcatalog";
+    Catalog catalog =
+        PolarisCatalog.builder()
+            .setType(Catalog.TypeEnum.INTERNAL)
+            .setName(catalogName)
+            .setStorageConfigInfo(
+                new AwsStorageConfigInfo(
+                    "arn:aws:iam::012345678901:role/jdoe", 
StorageConfigInfo.StorageTypeEnum.S3))
+            .setProperties(new CatalogProperties("s3://bucket1/"))

Review Comment:
   If we're not actually using S3, I think we may as well use `FILE`



##########
polaris-service/src/test/java/org/apache/polaris/service/admin/PolarisServiceImplIntegrationTest.java:
##########
@@ -1775,4 +1790,479 @@ public void testAssignListAndRevokeCatalogRoles() {
       assertThat(response).returns(204, Response::getStatus);
     }
   }
+
+  @Test
+  public void testCatalogAdminGrantAndRevokeCatalogRoles() {
+    // Create a PrincipalRole and a new catalog. Grant the catalog_admin role 
to the new principal
+    // role
+    String principalRoleName = "mypr33";
+    PrincipalRole principalRole1 = new PrincipalRole(principalRoleName);
+    createPrincipalRole(principalRole1);
+
+    String catalogName = "myuniquetestcatalog";
+    Catalog catalog =
+        PolarisCatalog.builder()
+            .setType(Catalog.TypeEnum.INTERNAL)
+            .setName(catalogName)
+            .setStorageConfigInfo(
+                new AwsStorageConfigInfo(
+                    "arn:aws:iam::012345678901:role/jdoe", 
StorageConfigInfo.StorageTypeEnum.S3))
+            .setProperties(new CatalogProperties("s3://bucket1/"))

Review Comment:
   nit: If we're not actually using S3, I think we may as well use `FILE`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to