nick-boss-tech commented on PR #5190: URL: https://github.com/apache/solr/pull/5190#issuecomment-6389701550
**Review TL/DR:** - P2: PicoCLI tests can run Commons CLI instead. The task no longer explicitly sets SOLR_PICOCLI=true. An inherited false makes both tasks test Commons CLI. Set both task modes explicitly. [Location](https://github.com/apache/solr/blob/f76e0589ef068d19675d4cfa44845cab1f029782/solr/packaging/build.gradle#L341-L343) - P3: Upgrade notes contradict the change. They still say the default CLI is unchanged and refer to snapshot spellings whose explanation was deleted. [Location](https://github.com/apache/solr/blob/f76e0589ef068d19675d4cfa44845cab1f029782/solr/solr-ref-guide/modules/upgrade-notes/pages/major-changes-in-solr-10.adoc#L39-L45) Connection to #5204: its presence-based assertion helper conflicts with #5190’s task configuration. Explicit false selects PicoCLI expectations; unset selects Commons CLI expectations despite the new default. The two PRs need consistent explicit mode selection. **Review Details:** 🤖 *AI text below and above* 🤖 *(reviewed by Nick Shanin)* @jaykay12 Thanks for clarifying the shared migration changelog in the earlier discussion. Keeping the entry under SOLR-17697 makes sense, and I confirmed that the SSL BATS fix from #5193 is already included in this branch. Reviewing `f76e0589ef06` alongside #5204, I found two additional issues: 1. **`integrationTestsPicocli` can silently exercise Commons CLI.** In `configureBatsTask`, the updated code sets `SOLR_PICOCLI=false` for the Commons CLI task but leaves the PicoCLI task’s value inherited. If the invoking environment already exports `SOLR_PICOCLI=false`, that value reaches the Java selector, so the task named `integrationTestsPicocli` also runs Commons CLI. Previously, its explicit `true` setting prevented this. Could both tasks set their intended value explicitly, for example `usePicocli ? 'true' : 'false'`? That would make their coverage independent of the invoking environment. A separate smoke test with the variable unset could verify that PicoCLI is actually the product default. This also connects to #5204: its new `assert_output_for_cli` helper treats any nonempty value, including `false`, as PicoCLI. After combining these changes, the Commons CLI task would select PicoCLI expectations. Conversely, an unset value in the PicoCLI task would select Commons CLI expectations. Explicit task values and matching boolean checks in the helper should address both sides. The restart-help skip in #5204 needs the same treatment. 2. **The 10.2 upgrade notes retain contradictory text.** The new PicoCLI subsection still says “The default command line interface is not changed.” It also refers to “The old spellings” after removing the snapshot-command explanation and replacement examples. Could the snapshot migration remain a separate subsection, with the default-switch explanation alongside it? This would also give the compatibility changes a clear home if they are moved here as Jan suggested. For verification, I would check that each named task selects its intended parser even when the invoking environment specifies the opposite value, followed by the relevant help/post coverage from #5204. These findings are from static inspection; I have not run Gradle or BATS. -- 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]
