jerryshao commented on PR #13849:
URL: https://github.com/apache/gravitino/pull/13849#issuecomment-6679255538
Thanks for the fix. I reviewed the diff and ran `TableFormatOutputIT`
locally on this branch (5 tests, all passing). Note that CI does not run
`:clients:cli:test`, so that local run is the only check of the CLI change so
far.
- `TableFormatOutputIT`: `jdbc-database=mydb` now matches the database in
`jdbc-url`, which the validation in `JdbcCatalog` requires.
`verify-on-create=false` is the documented way to register a catalog whose
backend is not reachable, so it fits a test that only checks CLI table output
against a fake host.
- `IcebergRESTAsyncPurgeIT`: it always starts a PostgreSQL container, so
`@Tag("gravitino-docker-test")` is consistent with the other container-based
ITs in the module.
One related gap, which does not block this PR: `IcebergAuthorizationIT` also
starts a PostgreSQL container in `@BeforeAll`
(`containerSuite.startPostgreSQLContainer(...)`) without the
`gravitino-docker-test` tag. Among its subclasses,
`IcebergRESTS3TokenAuthorizationIT` and `IcebergRESTGCSTokenAuthorizationIT`
are untagged too, while `IcebergNamespaceAuthorizationIT` and
`IcebergRESTRustFSTokenAuthorizationIT` are tagged. I read this from the code
and did not run it on a machine without Docker. It would be good to tag the
base class here, or to track it in a follow-up issue.
LGTM once Fork CI passes.
--
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]
