[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #4909: AWS: use pre-created IAM role for AssumeRole related integration tests

[GitBox <gi...@apache.org>]

amogh-jahagirdar commented on code in PR #4909:
URL: https://github.com/apache/iceberg/pull/4909#discussion_r886243443


##########
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java:
##########
@@ -117,7 +65,7 @@ public void testAssumeRoleGlueCatalog() throws Exception {
       Assert.assertEquals(AccessDeniedException.class, e.getClass());
     }
 
-    Namespace namespace = Namespace.of("allowed_" + UUID.randomUUID().toString().replace("-", ""));
+    Namespace namespace = Namespace.of("iceberg_aws_ci_" + UUID.randomUUID().toString().replace("-", ""));

Review Comment:
   Nit: iceberg_aws_ci_ maybe could be a static prefix? Not blocking though considering it was already like that 



##########
aws/src/integration/java/org/apache/iceberg/aws/TestAssumeRoleAwsClientFactory.java:
##########
@@ -129,41 +77,20 @@ public void testAssumeRoleGlueCatalog() throws Exception {
   }
 
   @Test
-  public void testAssumeRoleS3FileIO() throws Exception {
-    String bucketArn = "arn:aws:s3:::" + AwsIntegTestUtil.testBucketName();
-    iam.putRolePolicy(PutRolePolicyRequest.builder()
-        .roleName(roleName)
-        .policyName(policyName)
-        .policyDocument("{" +
-            "\"Version\":\"2012-10-17\"," +
-            "\"Statement\":[{" +
-            "\"Sid\":\"policy1\"," +
-            "\"Effect\":\"Allow\"," +
-            "\"Action\":\"s3:ListBucket\"," +
-            "\"Resource\":[\"" + bucketArn + "\"]," +
-            "\"Condition\":{\"StringLike\":{\"s3:prefix\":[\"allowed/*\"]}}} ,{" +
-            "\"Sid\":\"policy2\"," +
-            "\"Effect\":\"Allow\"," +
-            "\"Action\":\"s3:GetObject\"," +
-            "\"Resource\":[\"" + bucketArn + "/allowed/*\"]}]}")
-        .build());
-    waitForIamConsistency();
-
-    S3FileIO s3FileIO = new S3FileIO();
-    s3FileIO.initialize(assumeRoleProperties);
-    InputFile inputFile = s3FileIO.newInputFile("s3://" + AwsIntegTestUtil.testBucketName() + "/denied/file");
+  public void testAssumeRoleS3FileIO() {
+    S3Client s3Client = AwsClientFactories.from(assumeRoleProperties).s3();
     try {
-      inputFile.exists();
+      s3Client.getBucketAccelerateConfiguration(GetBucketAccelerateConfigurationRequest.builder()

Review Comment:
   I guess here we can use any method, so it does not matter but would getBucket just be less verbose to read? 



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org