paulrutter commented on PR #592: URL: https://github.com/apache/felix-dev/pull/592#issuecomment-6653585319
Thanks for this, the refactor itself looks good. I compared it with maven-compat 3.8.1, where getWagon(Repository) is just a lookup by protocol. Your replacement keeps the same behavior, including the case-insensitive server id match and decrypting the passwords. The tests are a nice addition too. I have a couple of proposals and would like your feedback on them. 1. Bundling wagon-file and wagon-http. They are compile scope, so every user of the plugin, including those who only run bundle:bundle, resolves wagon-http and its transitive HTTP stack. That seems like a lot for goals I suspect few people use. What do you think about leaving the providers out? On a Maven without them, the existing “Unsupported protocol” error would apply. Users who need it could add the providers as a build extension, and we could say so in the goal documentation. If you see a reason to bundle them, for example Maven 4 without the providers, I’m open to it. In that case, could wagon-file alone be enough, or could we scope the providers more narrowly? 2. Lock handling. lockFile(...) runs before the try/finally in ObrDeploy, ObrDeployFile and ObrRemoteClean. If the remote repository is already locked, it throws before the finally. The Wagon is then never disconnected or released, and with this PR it also never goes back to the container. I’d suggest moving the call inside the try, or wrapping it so disconnect() always runs. This exposure predates the PR. Would you be willing to include it here, or would you rather I do it as a follow-up? Two smaller points, optional: In connect(), a RuntimeException from the decrypter or the wagon skips releaseWagon(). Decryption problems (SettingsDecryptionResult.getProblems()) are ignored, so a bad master password shows up as a plain “Authentication failed”. maven-compat behaved the same way. Finally, a question: do you use the remote OBR goals yourself? The OBR code has seen no functional change since 2020, and I’m considering deprecating the goals. Knowing whether anyone relies on them, and whether you tested deploy-file and remote-clean, would help me decide. -- 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]
