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


##########
spec/polaris-management-service.yml:
##########
@@ -1056,6 +1056,12 @@ components:
               type: string
               description: endpoint for STS requests (optional). If not set, 
defaults to 'endpoint'.
               example: "https://sts.example.com:1234";
+            pathStyleAccess:
+              type: boolean
+              description: >- 
+                Whether S3 requests to files in this catalog should use 
'path-style addressing for buckets'.
+                Default: false.

Review Comment:
   Oh, I think that's okay to defer.
   
   But...
   > As long as pathStyleAccess is null in AwsStorageConfigInfo clients will 
not see it.
   
   That's only true if you don't create a new catalog. So take the following 
pseudocode:
   ```
   String catalog = ...createNewCatalog().toString
   assert(catalog == "{...}")
   ```
   
   We don't have a test like this in `CatalogSerializationTest`, but if we had 
one this PR would break it. Personally I don't think this really constitutes a 
breaking change, but I do think the proper way to handle a default in the API 
is with a `default` in the spec rather than a blurb in the description.



-- 
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-unsubscr...@polaris.apache.org

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

Reply via email to