mengw15 commented on code in PR #8713:
URL: https://github.com/apache/texera/pull/8713#discussion_r4114841347
##########
common/config/src/test/scala/org/apache/texera/common/config/StorageConfigSpec.scala:
##########
@@ -50,12 +51,23 @@ class StorageConfigSpec extends AnyFlatSpec with Matchers {
StorageConfig.ENV_CLEANUP_INTERVAL_MINUTES shouldBe
"STORAGE_CLEANUP_INTERVAL_MINUTES"
}
- "StorageConfig warehouse settings" should "default the feature to disabled
so merging changes nothing" in {
- // storage.warehouse.enabled is the kill switch for the whole per-user
warehouse
- // feature (#6870); this guards the safe default from silently flipping to
true.
- // Only assert when the env override is unset (e.g. in CI), since it would
win otherwise.
+ "StorageConfig warehouse settings" should "be on by default, and only with
the REST catalog" in {
+ // storage.warehouse.enabled is the switch for the whole per-user warehouse
+ // feature (#6870), on since #8712. Warehouses are Lakekeeper entities, so
the
+ // switch is conjoined with the catalog type: any other catalog keeps the
+ // feature off however the switch reads, which is what stops a deployment
+ // without Lakekeeper from being handed a feature it cannot serve.
+ // Only assert when the env override is unset, since it would win
otherwise.
+ // The resource default is asserted directly, not through
warehouseEnabled: CI's
+ // amber job pins the catalog to postgres, which would make the conjunction
+ // false either way and let a revert of the switch pass unnoticed.
+ ConfigFactory
+ .parseResources("storage.conf")
+ .resolve()
+ .getBoolean("storage.warehouse.enabled") shouldBe true
Review Comment:
You're right, and I reproduced it: with `STORAGE_WAREHOUSE_ENABLED=false`
the spec went to `Tests: succeeded 6, failed 1` before this change. `resolve()`
does apply the optional substitution, so the raw-config read was not
override-independent the way I assumed.
Moved it under the existing guard. It still does the job it was added for —
with the resource default mutated back to `false` the spec fails under
`STORAGE_ICEBERG_CATALOG_TYPE=postgres` as well (sbt exits 1), so a revert of
the switch is still caught in the configuration CI's amber job uses.
--
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]
