k-krawczyk commented on PR #27472: URL: https://github.com/apache/camel/pull/27472#issuecomment-6052579876
Pushed 755b315c with all six points addressed, plus the restart question fixed here rather than deferred. **1. `--background` never checked the properties.** The right catch, and the worst of them. The options were read inside `doRun`, which the background path never reaches, so the parent printed a pid and returned 0 while the child exited 1 where nobody was looking. The pairs are now parsed in `run()`, before the background branch, so the complaint lands in the terminal the user is typing in. **2. Nothing was cleared when the start failed.** Setting the properties is now separate from starting the service: `doRun` applies them, calls a new `startService`, and restores in a `finally`, so it no longer matters whether `initialize()` threw or the service never got that far. The earlier cleanup only ran once `shutdownInfra` was reached. **3 and oscerd's first point: the empty value.** Both right, and the description was the thing that was wrong-but-truthful: `separator < 1` catches a missing `=` and an empty name, not `foo=`. It is refused now, with a test over all three malformed shapes. **4. The documented container limits.** My mistake, and thanks for catching it: `ollama.container.cpu.count` and `ollama.container.memory.limit` have constants in `OllamaProperties` and commented-out defaults in `container.properties`, but nothing reads them - only `ENABLE_GPU` is read. I took the constants as proof the settings existed. Gone from the docs. **oscerd's second point: a pre-existing `-D` was cleared, not restored.** Applying a property now records what it replaced, and the restore puts that value back, or removes the property when there was none. Two tests cover it. In a JVM started with `-Dollama.model=X`, a run with `--property ollama.model=Y` leaves `X` behind it. **The open question about `camel infra restart`.** Fixed here - it turned out to be three lines. `InfraRestart` builds its own `InfraRun` and already makes you repeat `--port`, so `--property` behaves the same way, and the generated restart docs picked up the option. One thing nobody raised: the field was named `properties`, which `doRun` shadows with its own `Map<String, Object> properties` for the service data. Renamed to `serviceProperties`; the option names are unchanged. `InfraRunPropertyTest` is at six cases and the whole `Infra*Test` set passes (25 tests), with `-Psourcecheck` clean. Still not exercised against a real container pull - the tests drive the mapping, not a download. _Reported by Claude Code on behalf of Karol Krawczyk_ -- 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]
