Octavi00 commented on PR #10975: URL: https://github.com/apache/gravitino/pull/10975#issuecomment-4598238127
Hello @roryqi, i made some big changes as requested, and I will list most of them so it's easier to read. Instead of using an initialize method as previously done so in Iceberg, I went with a withTLSConfigurer method. This required large modifications and led to some more files needing adjustments. To start, due to the fact that a user makes a gravitino client base, and the flow is base -> adminClient or client -> http - All 4 files had the .withTlsConfigurer() function added. To maintain all prior test cases + ensure existing code is unaffected: - The same default constructor was kept by initializing the new TLSConfigurer field to null * Possible future revisions/additions might not want to keep making several different constructors, but rather an object that holds configurators such as TLS within it to avoid large, redundant code. Regardless, making another constructor wasn't reasonably avoidable in this case. -E2E was locally tested using a custom test file and a custom test, which can be seen below, and passes the expected behaviour <img width="2560" height="1403" alt="Screenshot 2026-05-28 224619" src="https://github.com/user-attachments/assets/f30cae76-74b7-4571-b077-54c6a14641d5"; /> -gravitino.conf.template was updated to make an easier user experience when setting up HTTPS when using TLS - Two simple TLS unit tests were made inside TestHTTPClient to replace prior unit tests * Maybe will want similar ones within the 3 gravitino objects for new constructors, but all prior behaviour remains the same Thank you -- 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]
