[GitHub] [beam] hmehrotra commented on a diff in pull request #25246: Added Role-based access control integration tests for Spanner Change …

["hmehrotra (via GitHub)" <gi...@apache.org>]

hmehrotra commented on code in PR #25246:
URL: https://github.com/apache/beam/pull/25246#discussion_r1093664151


##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/it/ChangeStreamTestPipelineOptions.java:
##########
@@ -37,8 +37,20 @@ public interface ChangeStreamTestPipelineOptions extends IOTestPipelineOptions,
   void setInstanceId(String value);
 
   @Description("Database ID prefix to write to in Spanner")
-  @Default.String("changestream")
+  @Default.String("cs_primary")
   String getDatabaseId();
 
   void setDatabaseId(String value);
+
+  @Description("Role-Based Access Control Database ID prefix to write to in Spanner")
+  @Default.String("cs_rbac")
+  String getRbacDatabaseId();

Review Comment:
   Do we need a separate database for RBAC? Why can't we use the same DatabaseId? You would still need to create a separate DB client for rbac.



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/it/IntegrationTestEnv.java:
##########
@@ -48,18 +49,44 @@ public class IntegrationTestEnv extends ExternalResource {
   private static final String METADATA_TABLE_NAME_PREFIX = "TestMetadata";
   private static final String SINGERS_TABLE_NAME_PREFIX = "Singers";
   private static final String CHANGE_STREAM_NAME_PREFIX = "SingersStream";
+  private static final String DATABASE_ROLE = "test_role";
+  private List<String> databases;
   private List<String> changeStreams;
   private List<String> tables;
 
   private String projectId;
   private String instanceId;
   private String databaseId;
+  private String rbacDatabaseId;
+  private String metadataDatabaseId;
   private String metadataTableName;
   private Spanner spanner;
   private final String host = "https://spanner.googleapis.com";
   private DatabaseAdminClient databaseAdminClient;
   private DatabaseClient databaseClient;
+  private DatabaseClient rbacDatabaseClient;
   private boolean isPostgres;
+  public boolean createRbacDatabase;
+  public boolean useSeparateMetadataDb;
+
+  public IntegrationTestEnv withRbacDatabase() {
+    this.createRbacDatabase = true;

Review Comment:
   Use 'isRbac' as the parameter name to be consistent with 'isPostgres'?



##########
sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/changestreams/it/IntegrationTestEnv.java:
##########
@@ -70,154 +97,186 @@ protected void before() throws Throwable {
         Optional.ofNullable(options.getProjectId())
             .orElseGet(() -> options.as(GcpOptions.class).getProject());
     instanceId = options.getInstanceId();
-    databaseId = generateDatabaseName(options.getDatabaseId());
+    generateDatabaseIds(options);
     spanner =
         SpannerOptions.newBuilder().setProjectId(projectId).setHost(host).build().getService();
     databaseAdminClient = spanner.getDatabaseAdminClient();
     metadataTableName = generateTableName(METADATA_TABLE_NAME_PREFIX);
 
+    databases = new ArrayList<>();
     recreateDatabase(databaseAdminClient, instanceId, databaseId, isPostgres);
-
     databaseClient = spanner.getDatabaseClient(DatabaseId.of(projectId, instanceId, databaseId));
+    databases.add(databaseId);
+
+    if (createRbacDatabase) {
+      recreateDatabase(databaseAdminClient, instanceId, rbacDatabaseId, isPostgres);
+      rbacDatabaseClient =
+          spanner.getDatabaseClient(DatabaseId.of(projectId, instanceId, rbacDatabaseId));
+      databases.add(rbacDatabaseId);
+    }
+    if (useSeparateMetadataDb) {

Review Comment:
   Do we need a separate param for useSeparateMetadataDb? We could just check if (isRbac) here. Right?



-- 
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: github-unsubscribe@beam.apache.org

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