adutra commented on code in PR #987:
URL: https://github.com/apache/polaris/pull/987#discussion_r1965569292
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java:
##########
@@ -118,25 +118,26 @@ public class PolarisApplicationIntegrationTest {
private static ClientCredentials clientCredentials;
private static ClientPrincipal admin;
private static String authToken;
+ private static URI baseLocation;
private String principalRoleName;
private String internalCatalogName;
@BeforeAll
- public static void setup(PolarisApiEndpoints apiEndpoints, ClientPrincipal
adminCredentials)
- throws IOException {
+ public static void setup(
+ PolarisApiEndpoints apiEndpoints, ClientPrincipal adminCredentials,
@TempDir Path tempDir) {
endpoints = apiEndpoints;
client = polarisClient(endpoints);
realm = endpoints.realmId();
admin = adminCredentials;
clientCredentials = adminCredentials.credentials();
authToken = client.obtainToken(clientCredentials);
-
- testDir = Path.of("build/test_data/iceberg/" + realm);
- FileUtils.deleteQuietly(testDir.toFile());
- Files.createDirectories(testDir);
-
managementApi = client.managementApi(clientCredentials);
+ baseLocation =
+ Optional.ofNullable(System.getenv("INTEGRATION_TEST_TEMP_DIR"))
+ .map(URI::create)
+ .orElse(tempDir.toUri())
+ .resolve(realm + "/");
Review Comment:
> This may have unexpected behaviour... IIRC s3://bucket/a/b
.resolve("realm/") will yield s3://bucket/a/realm/... Did you mean that?
No, of course; the expectation is that `INTEGRATION_TEST_TEMP_DIR` must end
with a trailing slash.
> It might be best to do a simple string append and then call
URI.normalize().
Yep, let me change that.
> Also, some valid S3 locations do not pass URI validation checks [...] I
think it might be best to stick with string concat
I remember the pain, but OTOH I dislike writing stringly-typed code. Do you
think the issues we had in Nessie would manifest here? Honestly I expect these
URIs to be as simple as `s3://test-bucket`, so they should be parsable. And
also, as you said, Polaris is not ready yet to handle those problematic S3
locations.
--
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]
