ghprb-plugin
ghprb-plugin copied to clipboard
jenkinsci
Use RateLimitHandler.FAIL as GitHub rate limiter for fast fail behavior
Hello, could anybody from the maintainers take a look? This fix is very important for our project, thank you!
What is the reasoning for this change? Why do we need this? Whats it the non-obvious benefit?
Also this is a rather breaking change, so we will not be able to merge it like this. We would need to put this either behind a configuration or env variable.
Okay, I see the point of this change.
@samrocketman Shall we merge it like this (breaking behavior in a extreme case [api limit reached]) or should we put this behind a env variable?
We could merge this but we need a warning in the CHANGELOG before release. We also need to warn the update center about breaking changes which is exposed to admins before upgrading.
If we merge it as-is, then we need to do two things before releasing.
- Add a warning to the upgrade notes in the CHANGELOG describing the change in behavior.
- Add a compatibility warning to the Jenkins Update Center so that admins are compelled to go read the release notes. Normally, this is done for configuration compatibility issues but this is chsnging a core behavior (albeit fixing a UI bug?). Here’s an example of a compatibility warning for the update center (compatible since would be the last stable release of GHPRB) https://github.com/jenkinsci/pipeline-multibranch-defaults-plugin/commit/a2c41c2b96dec498862e20ad9c4f3a70799c11cd
I would like a 3rd expert opinion from @jenkinsci/code-reviewers ; please review and let us know anything we might have missed (compatibility or otherwise).
If we merge it as-is, then we need to do two things before releasing.
- Add a warning to the upgrade notes in the CHANGELOG describing the change in behavior.
- Add a compatibility warning to the Jenkins Update Center so that admins are compelled to go read the release notes. Normally, this is done for configuration compatibility issues but this is chsnging a core behavior (albeit fixing a UI bug?). Here’s an example of a compatibility warning for the update center (compatible since would be the last stable release of GHPRB) https://github.com/jenkinsci/pipeline-multibranch-defaults-plugin/commit/a2c41c2b96dec498862e20ad9c4f3a70799c11cd
I would like a 3rd expert opinion from @jenkinsci/code-reviewers ; please review and let us know anything we might have missed (compatibility or otherwise).
If we merge it as-is, then we need to do two things before releasing.
- Add a warning to the upgrade notes in the CHANGELOG describing the change in behavior.
- Add a compatibility warning to the Jenkins Update Center so that admins are compelled to go read the release notes. Normally, this is done for configuration compatibility issues but this is chsnging a core behavior (albeit fixing a UI bug?). Here’s an example of a compatibility warning for the update center (compatible since would be the last stable release of GHPRB) https://github.com/jenkinsci/pipeline-multibranch-defaults-plugin/commit/a2c41c2b96dec498862e20ad9c4f3a70799c11cd
I would like a 3rd expert opinion from @jenkinsci/code-reviewers ; please review and let us know anything we might have missed (compatibility or otherwise).
Changed based on your feedback.
I'd take this as a fix of bad behavior which breaks Jenkins core functionality rather than a compatibility break/issue of the plug-in itself. However no strict preference for this hard change, would be an option as well (can anybody want to use the old blocking behavior than?).
Guys, is there anything else what can I do for going further with this PR? As you can see there are more people who are affected by this defected behavior which breaks the Jenkins core functionality.
@samrocketman, we are having issues caused by this so I would really to see this one fixed. The announcements are surely doable, I see Pavel has already implemented both of them. Are we good to go? Thanks
EDIT: the checkstyle violations that are causing this build to fail are unrelated....
@bjoernhaeuser @samrocketman Hey, we put this change on top of 1.42 and build own version of ghprb plugin. The change fixes the issue and plugin works fine (for more than a moth) on our production system that is responsible for PR build in kiegroup projects [1]. It seems to me that it's safe to merge.
[1] https://github.com/kiegroup
I would like this merged as well as i just had this issue happen as well, for my jenkins
@bjoernhaeuser Could you please merge this 🙏? Our builds are affected by this issue as well.
@conf I can take a look. Looks like there's failing tests so I'll have to pull it down locally and try it out. I've got some family coming over tonight so I likely won't get to tonight (EST). I can take a look at it tomorrow after I'm done with work. I'll likely add a commit to this with fixes (checkstyle, resolved conflicts etc).
@samrocketman Just a kind reminder to look into this. I've rebased against master, resolved the conflict, and pushed to my fork here https://github.com/jenkinsci/ghprb-plugin/compare/master...conf:JENKINS-19123-RateLimitHandler, if it helps somehow. The tests seem to pass, I'm running them in a docker image with java 8 and maven 3.6.3:
♥ docker run -it -v maven-repo:/root/.m2 -v $PWD:/usr/src/mymaven -w /usr/src/mymaven maven:3.6.3-jdk-8 mvn clean test
[INFO] Scanning for projects...
[WARNING] The POM for org.jenkins-ci.tools:maven-hpi-plugin:jar:2.1 is missing, no dependency information available
[WARNING] Failed to build parent project for org.jenkins-ci.plugins:ghprb:hpi:1.42.2-SNAPSHOT
[INFO]
[INFO] --------------------< org.jenkins-ci.plugins:ghprb >--------------------
[INFO] Building GitHub Pull Request Builder 1.42.2-SNAPSHOT
[INFO] --------------------------------[ hpi ]---------------------------------
Downloading from repo.jenkins-ci.org: https://repo.jenkins-ci.org/public/org/apache/maven/plugins/maven-clean-plugin/3.0.0/maven-clean-plugin-3.0.0.pom
Downloaded from repo.jenkins-ci.org: https://repo.jenkins-ci.org/public/org/apache/maven/plugins/maven-clean-plugin/3.0.0/maven-clean-plugin-3.0.0.pom (4.8 kB at 3.5 kB/s)
Downloading from repo.jenkins-ci.org: https://repo.jenkins-ci.org/public/org/apache/maven/plugins/maven-clean-plugin/3.0.0/maven-clean-plugin-3.0.0.jar
Downloaded from repo.jenkins-ci.org: https://repo.jenkins-ci.org/public/org/apache/maven/plugins/maven-clean-plugin/3.0.0/maven-clean-plugin-3.0.0.jar (31 kB at 60 kB/s)
[INFO]
[INFO] --- maven-clean-plugin:3.0.0:clean (default-clean) @ ghprb ---
[INFO] Deleting /usr/src/mymaven/target
[INFO]
[INFO] --- maven-hpi-plugin:2.1:validate (default-validate) @ ghprb ---
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0-M1:display-info (display-info) @ ghprb ---
[INFO] Maven Version: 3.6.3
[INFO] JDK Version: 1.8.0_252 normalized as: 1.8.0-252
[INFO] OS Info: Arch: amd64 Family: unix Name: linux Version: 4.19.76-linuxkit
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0-M1:enforce (display-info) @ ghprb ---
[INFO] Ignoring requireUpperBoundDeps in com.google.guava:guava
[INFO] Ignoring requireUpperBoundDeps in com.google.code.findbugs:jsr305
[INFO]
[INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ ghprb ---
[INFO]
[INFO] --- maven-localizer-plugin:1.24:generate (default) @ ghprb ---
[INFO]
[INFO] >>> maven-javadoc-plugin:2.10.4:javadoc (default) > generate-sources @ ghprb >>>
[INFO]
[INFO] --- maven-hpi-plugin:2.1:validate (default-validate) @ ghprb ---
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0-M1:display-info (display-info) @ ghprb ---
[INFO] Maven Version: 3.6.3
[INFO] JDK Version: 1.8.0_252 normalized as: 1.8.0-252
[INFO] OS Info: Arch: amd64 Family: unix Name: linux Version: 4.19.76-linuxkit
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0-M1:enforce (display-info) @ ghprb ---
[INFO] Ignoring requireUpperBoundDeps in com.google.guava:guava
[INFO] Ignoring requireUpperBoundDeps in com.google.code.findbugs:jsr305
[INFO]
[INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ ghprb ---
[INFO]
[INFO] --- maven-localizer-plugin:1.24:generate (default) @ ghprb ---
[INFO]
[INFO] <<< maven-javadoc-plugin:2.10.4:javadoc (default) < generate-sources @ ghprb <<<
[INFO]
[INFO]
[INFO] --- maven-javadoc-plugin:2.10.4:javadoc (default) @ ghprb ---
[INFO]
[INFO] --- maven-resources-plugin:3.0.2:resources (default-resources) @ ghprb ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] Copying 59 resources
[INFO]
[INFO] --- maven-compiler-plugin:3.5.1:compile (default-compile) @ ghprb ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 53 source files to /usr/src/mymaven/target/classes
[INFO] /usr/src/mymaven/src/main/java/org/jenkinsci/plugins/ghprb/GhprbBuilds.java: Some input files use or override a deprecated API.
[INFO] /usr/src/mymaven/src/main/java/org/jenkinsci/plugins/ghprb/GhprbBuilds.java: Recompile with -Xlint:deprecation for details.
[INFO] /usr/src/mymaven/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java: Some input files use unchecked or unsafe operations.
[INFO] /usr/src/mymaven/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java: Recompile with -Xlint:unchecked for details.
[INFO]
[INFO] --- access-modifier-checker:1.8:enforce (default-enforce) @ ghprb ---
[INFO]
[INFO] --- animal-sniffer-maven-plugin:1.15:check (check) @ ghprb ---
[INFO] Resolved signature org.codehaus.mojo.signature:java17 version as 1.0 from dependencyManagement
[INFO] Checking unresolved references to org.codehaus.mojo.signature:java17:1.0
[INFO]
[INFO] --- maven-hpi-plugin:2.1:insert-test (default-insert-test) @ ghprb ---
[INFO]
[INFO] --- gmaven-plugin:1.5-jenkins-3:generateTestStubs (test-in-groovy) @ ghprb ---
[INFO] No sources found for Java stub generation
[INFO]
[INFO] --- maven-resources-plugin:3.0.2:testResources (default-testResources) @ ghprb ---
[INFO] Using 'UTF-8' encoding to copy filtered resources.
[INFO] skip non existing resourceDirectory /usr/src/mymaven/src/test/resources
[INFO]
[INFO] --- maven-compiler-plugin:3.5.1:testCompile (default-testCompile) @ ghprb ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 18 source files to /usr/src/mymaven/target/test-classes
[INFO] /usr/src/mymaven/src/test/java/org/jenkinsci/plugins/ghprb/manager/impl/GhprbDefaultBuildManagerTest.java: Some input files use or override a deprecated API.
[INFO] /usr/src/mymaven/src/test/java/org/jenkinsci/plugins/ghprb/manager/impl/GhprbDefaultBuildManagerTest.java: Recompile with -Xlint:deprecation for details.
[INFO] /usr/src/mymaven/src/test/java/org/jenkinsci/plugins/ghprb/GhprbIT.java: /usr/src/mymaven/src/test/java/org/jenkinsci/plugins/ghprb/GhprbIT.java uses unchecked or unsafe operations.
[INFO] /usr/src/mymaven/src/test/java/org/jenkinsci/plugins/ghprb/GhprbIT.java: Recompile with -Xlint:unchecked for details.
[INFO]
[INFO] --- maven-hpi-plugin:2.1:test-hpl (default-test-hpl) @ ghprb ---
[INFO] Generating /usr/src/mymaven/target/test-classes/the.hpl
[INFO]
[INFO] --- maven-hpi-plugin:2.1:resolve-test-dependencies (default-resolve-test-dependencies) @ ghprb ---
[INFO]
[INFO] --- gmaven-plugin:1.5-jenkins-3:testCompile (test-in-groovy) @ ghprb ---
[INFO] No sources found to compile
[INFO]
[INFO] --- maven-surefire-plugin:2.16:test (default-test) @ ghprb ---
[INFO] Surefire report directory: /usr/src/mymaven/target/surefire-reports
[INFO] Using configured provider org.apache.maven.surefire.junitcore.JUnitCoreProvider
[INFO] parallel='none', perCoreThreadCount=true, threadCount=0, useUnlimitedThreads=false, threadCountSuites=0, threadCountClasses=0, threadCountMethods=0
-------------------------------------------------------
T E S T S
-------------------------------------------------------
Running org.jvnet.hudson.test.JellyTestSuiteBuilder$JellyCheck
Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.573 sec - in org.jvnet.hudson.test.JellyTestSuiteBuilder$JellyCheck
Running org.jvnet.hudson.test.PluginAutomaticTestBuilder$OtherTests
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.976 sec - in org.jvnet.hudson.test.PluginAutomaticTestBuilder$OtherTests
Running org.jvnet.hudson.test.junit.FailedTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.065 sec - in org.jvnet.hudson.test.junit.FailedTest
Running org.jenkinsci.plugins.ghprb.GeneralTest
Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.333 sec - in org.jenkinsci.plugins.ghprb.GeneralTest
Running org.jenkinsci.plugins.ghprb.GhprbBuildsTest
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.368 sec - in org.jenkinsci.plugins.ghprb.GhprbBuildsTest
Running org.jenkinsci.plugins.ghprb.GhprbPullRequestMergeTest
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.848 sec - in org.jenkinsci.plugins.ghprb.GhprbPullRequestMergeTest
Running org.jenkinsci.plugins.ghprb.GhprbPullRequestTest
Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.182 sec - in org.jenkinsci.plugins.ghprb.GhprbPullRequestTest
Running org.jenkinsci.plugins.ghprb.GhprbRepositoryTest
Tests run: 11, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 10.904 sec - in org.jenkinsci.plugins.ghprb.GhprbRepositoryTest
Running org.jenkinsci.plugins.ghprb.GhprbRootActionTest
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 25.894 sec - in org.jenkinsci.plugins.ghprb.GhprbRootActionTest
Running org.jenkinsci.plugins.ghprb.GhprbTriggerTest
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 4.784 sec - in org.jenkinsci.plugins.ghprb.GhprbTriggerTest
Running org.jenkinsci.plugins.ghprb.extensions.build.GhprbCancelBuildsOnUpdateTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.929 sec - in org.jenkinsci.plugins.ghprb.extensions.build.GhprbCancelBuildsOnUpdateTest
Running org.jenkinsci.plugins.ghprb.extensions.status.GhprbSimpleStatusTest
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.19 sec - in org.jenkinsci.plugins.ghprb.extensions.status.GhprbSimpleStatusTest
Running org.jenkinsci.plugins.ghprb.manager.configuration.JobConfigurationTest
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.002 sec - in org.jenkinsci.plugins.ghprb.manager.configuration.JobConfigurationTest
Running org.jenkinsci.plugins.ghprb.manager.factory.GhprbBuildManagerFactoryUtilTest
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.996 sec - in org.jenkinsci.plugins.ghprb.manager.factory.GhprbBuildManagerFactoryUtilTest
Running org.jenkinsci.plugins.ghprb.manager.impl.GhprbDefaultBuildManagerTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 11.947 sec - in org.jenkinsci.plugins.ghprb.manager.impl.GhprbDefaultBuildManagerTest
Running org.jenkinsci.plugins.ghprb.manager.impl.downstreambuilds.BuildFlowBuildManagerTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 21.155 sec - in org.jenkinsci.plugins.ghprb.manager.impl.downstreambuilds.BuildFlowBuildManagerTest
Results :
Tests run: 76, Failures: 0, Errors: 0, Skipped: 0
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 04:07 min
[INFO] Finished at: 2020-05-24T11:15:34Z
[INFO] ------------------------------------------------------------------------
Is there anything else I can help with?
@conf I can't seem to wrangle checkstyle into working which would block release. Here's an example failure.
mvn clean test package
$ mvn clean test package
... log snipped ...
[INFO] --- maven-checkstyle-plugin:2.17:check (validate) @ ghprb ---
[INFO] There are 9 errors reported by Checkstyle 6.7 with /home/sam/git/github/ghprb-plugin/checkstyle.xml ruleset.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[1] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - java.util.*.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[9,44] (whitespace) WhitespaceAround: '+' is not preceded with whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[9,45] (whitespace) WhitespaceAround: '+' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[11,30] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[12,33] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[13,32] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[14,38] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[15,42] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[ERROR] target/generated-test-sources/injected/InjectedTest.java:[16,32] (whitespace) WhitespaceAfter: ',' is not followed by whitespace.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:58 min
[INFO] Finished at: 2020-07-16T01:03:05-04:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (validate) on project ghprb: You have 9 Checkstyle violations. -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException
I found https://stackoverflow.com/questions/2362652/excluding-classes-in-maven-checkstyle-plugin-reports
However, I can't seem to get it to work. For now, this is as far as I can go tonight since it's late my local time. I'll check back tomorrow.
environment
Here's my environment (click to expand)
$ java -version
openjdk version "1.8.0_252"
OpenJDK Runtime Environment (build 1.8.0_252-8u252-b09-1~18.04-b09)
OpenJDK 64-Bit Server VM (build 25.252-b09, mixed mode)
$ mvn -version
Apache Maven 3.6.0
Maven home: /usr/share/maven
Java version: 1.8.0_252, vendor: Private Build, runtime: /usr/lib/jvm/java-8-openjdk-amd64/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.3.0-62-generic", arch: "amd64", family: "unix"
$ head /etc/issue
Ubuntu 18.04.4 LTS
@samrocketman Thanks for looking into this. I've missed the package target and didn't notice the codestyle errors. I've applied this answer for SO and it seems to work, please, have a look.
