schedin opened a new pull request, #13: URL: https://github.com/apache/maven-jarsigner-plugin/pull/13
This pull request implements https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-41 that introduces a retry feature for when jar signing fails (typically for network issues). It also implements a delay feature between retries. In this pull request I have: 1. Used the pull request https://github.com/apache/maven-jarsigner-plugin/pull/1 by @lasselindqvist as base along with the branch https://github.com/apache/maven-jarsigner-plugin/tree/MJARSIGNER-41 by @slachiewicz. 2. Also integrated the suggested change by https://github.com/apache/maven-jarsigner-plugin/pull/3 by @bdt-stru. 3. Fixed https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-68 4. Written a huge number of tests. I thought a lot about whether both signing and verification should have the retry feature. In the end I felt the code would be better if only the signing had the retry feature. I’m unsure if any verification may fail due to external circumstances. Perhaps if external Certificate Revocation Lists (CRLs) are used? But I’m unsure if jarsigner performs such a check to an external network resource? I opted to use `@since 3.1.0` declaration(s) for the added parameters. Feel free to change/suggest other values for this! I spent some time thinking about if a linear or exponential backoff algorithm for sleeping should be used. In the end I made a simple exponential backoff implementation with only one configurable parameter (maxRetryDelay). It would be possible to also add configuration for initalDelay (hard coded to 1 second) and multiplier (hard coded to 2), but I choose not to do this to keep things simple. I considered implementing support for multiple tsa lists. It was suggested by Thorsten Meinl as a patch to https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-59 and also by @jcompagner in https://github.com/apache/maven-jarsigner-plugin/pull/1#issuecomment-1412344998. I think this could be a good idea, but I did not want to “pollute” my pull request. If it was to be implemented I think the way forward is to change type of JarsignerSignMojo.tsa from a String to String[] and let Maven default array handling handle this (it will then handle both using comma separation, or nested XML tags). But my guess is that an adding of tsa list would “force” to also use a list for -tsacert and also the non-existent parameter -tsapolicyid? Because tsa, tsacert and tsapolicyid all belong together as a triplet. I implemented varargs for the `getMessage()` methods to make them cleaner. With the combination of what bdt-stru and lasselindqvist added I ended up adding one log and one obscure exception (that will not happen often) that does not have internationalization (see `JarsignerSignMojo.defaultWaitStrategy()`). My own feeling is that if the Mojo configuration site documentation does not have internationalization, is it that important that some corner case log messages also does not have it? I spend a lot of time writing test cases. My motivation for this is that I like tests and that I plan to continue working to implement https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-72. I suspect I will need some refactoring and a safety net when I begin with that. While writing test case I saw that maven-jarsigner-plugin was missing support for the `-signedjar` parameter. But I did not have a direct need to have this feature, so I did not implement it. I cannot find any ticket for this on https://issues.apache.org/jira/projects/MJARSIGNER/issues so I guess there is no high need for it. I tried using maven Maven Plugin Testing Harness (https://maven.apache.org/plugin-testing/maven-plugin-testing-harness/). I spent many hours on it. I felt that the documentation/implementation was not mature enough to use. I did not find any good way of injecting custom instances of JarSigner/MavenSession/MavenProject. It is possible to make Maven Plugin Testing Harness instantiate a class (that is runing the default constructor). But that was not enough for me: I needed to tailor the instances. The way to make it instantiate custom classes was too cumbersome in my opinion (that I had to go via a pom.xml file on disk). It was possible to create via Java methods only, but those methods were not documented. All in all, I decided to create my own mini-test hardness framework (see MojoTestCreator.java and PluginXmlParser.java). Since slachiewicz, in the integration branch https://github.com/apache/maven-jarsigner-plugin/tree/MJARSIGNER-41, tried to use jacoco for code coverage I completed the configuration so that Jacoco will output HTML reports in target/site/jacoco/index.html. Unfortunately, I don’t have access or don’t fully understand how the CI (Jenkins?) server https://ci-maven.apache.org/job/Maven/job/maven-box/job/maven-jarsigner-plugin/ works to know what good values are for a code coverage report to be shown on a job build page. Just as Elliotte Rusty has noted in https://issues.apache.org/jira/projects/MJARSIGNER/issues/MJARSIGNER-68 the integration test /src/it/verify-fail/pom.xml fails when I executed it (mvn clean install -P+run-its). I have made a fix for this and documented how I did to generate the tampered.jar in src/it/verify-fail/pom.xml. -- 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: issues-unsubscr...@maven.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
