bitflicker64 commented on PR #3119: URL: https://github.com/apache/hugegraph/pull/3119#issuecomment-5152791382
Follow-up push (5268091) after another review pass. No behaviour change — 604 tests, 0 failures, checkstyle clean. - The entrypoint comment said the accepted spellings are "the ones HugeConfig accepts (BooleanUtils)". That is ambiguous, and it misled a bot reviewer into reporting the `0`/`1` test as a false green. Conversion actually runs through commons-configuration 1.x `PropertyConverter` → commons-lang **2.6** `BooleanUtils`, which rejects `0` and `1`; commons-lang3 would accept them. The comment now names the path. - `docker/README.md` said "With `false`, the entrypoint deliberately never writes `docker/init_complete`". True for the environment variable, not for the property set directly in a mounted config. Reworded, with the limitation stated. - `InitStoreConfigTest` moved out of the `/* core */` group in `UnitTestSuite` into its own `/* cmd */` group, and its comments trimmed toward the density of the surrounding suite. Two things I would rather raise myself than have you find them: **`checkAdminBootstrapReachable` may be in the wrong class.** `StandardAuthenticator.setup()` already does the same resolution — `scanGraphsDir(GRAPHS)`, look up `auth.graph_store`, fail naming both options — and `StandardAuthenticator.initAdminUserIfNeeded(String)` is the existing static config-path entry point that `InitStore` already calls. So the check arguably belongs beside that code rather than in `InitStore`, which as written reaches into auth internals and repeats the auth-graph lookup. I kept it in `InitStore` because that is where the skip decision is made and where the exit status has to be produced, but I will move it if you would rather. **One branch is effectively unreachable and untested.** `ConfigUtil.scanGraphsDir` throws when the graphs directory is absent, so for `usePD=true` + built-in auth + no local `conf/graphs`, the failure surfaces as `scanGraphsDir`'s generic message rather than "auth graph 'x' has no local configuration". The `path == null` branch only fires when the directory exists but lacks the named graph. I can add a test for that case, or fold the two messages together. Also for ordering: #3132 (the Helm chart) injects `HG_SERVER_INIT_STORE_ENABLED=false` and depends on this PR's env mapping, so it should land after this one. -- 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]
