plusplusjiajia commented on code in PR #3120: URL: https://github.com/apache/iceberg-python/pull/3120#discussion_r2969376326
########## tests/catalog/test_rest.py: ########## Review Comment: > ideally we should test against an integration test before merging. unit tests can only do so much Agreed, integration tests would definitely give more confidence here. Currently there are no SigV4-related integration tests in the project — the existing integration tests under tests/integration/ use a docker-compose based REST catalog without SigV4 enabled. The [Java implementation also tests SigV4](https://github.com/apache/iceberg/blob/a89f1f9aa/aws/src/test/java/org/apache/iceberg/aws/TestRESTSigV4Signer.java) with rather than integration tests against a real service. One challenge with adding SigV4 integration tests to CI is that it would require real AWS credentials (access key / secret key), which might not be straightforward to set up in the open-source CI environment. On my end, I've been able to verify this change against a REST catalog server built with the Iceberg Java SDK. I'd really appreciate any suggestions you might have on how best to approach this. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
