Don't let the build fail if the commit doesn't exist in upstream #111

kohsuke · 2016-03-07T23:28:50Z

PR builds and other merge activities can create a new tip of the repository that's not in the github repository. This plugin causes the build to fail in such a case today, which is undesirable. We'd rather just have the build to succeed without a commit status.

This is the stack trace you get when that happens:

ERROR: Step ‘Set build status on GitHub commit’ aborted due to exception: 
java.io.FileNotFoundException: {"message":"No commit found for SHA: bb077596591672c8348ace0f8ddb2a13df2e0892","documentation_url":"https://developer.github.com/v3/repos/statuses/"}
    at org.kohsuke.github.Requester.handleApiError(Requester.java:527)
    at org.kohsuke.github.Requester._to(Requester.java:257)
    at org.kohsuke.github.Requester.to(Requester.java:203)
    at org.kohsuke.github.GHRepository.createCommitStatus(GHRepository.java:854)
    at com.cloudbees.jenkins.GitHubCommitNotifier.updateCommitStatus(GitHubCommitNotifier.java:142)
    at com.cloudbees.jenkins.GitHubCommitNotifier.perform(GitHubCommitNotifier.java:111)
    at hudson.tasks.BuildStepCompatibilityLayer.perform(BuildStepCompatibilityLayer.java:75)
    at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
    at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:795)
    at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:736)
    at hudson.model.Build$BuildExecution.post2(Build.java:185)
    at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:681)
    at hudson.model.Run.execute(Run.java:1766)
    at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:408)
Caused by: java.io.FileNotFoundException: https://api.github.com/repos/cloudbees/blueocean/statuses/bb077596591672c8348ace0f8ddb2a13df2e0892
    at com.squareup.okhttp.internal.huc.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:240)
    at com.squareup.okhttp.internal.huc.DelegatingHttpsURLConnection.getInputStream(DelegatingHttpsURLConnection.java:210)
    at com.squareup.okhttp.internal.huc.HttpsURLConnectionImpl.getInputStream(HttpsURLConnectionImpl.java:25)
    at org.kohsuke.github.Requester.parse(Requester.java:483)
    at org.kohsuke.github.Requester._to(Requester.java:236)
    ... 14 more

michaelneale · 2016-03-07T23:31:13Z

src/main/java/com/cloudbees/jenkins/GitHubCommitNotifier.java

+                    // PR builds and other merge activities can create a merge commit that
+                    // doesn't exist in the upstream. Don't let the build fail
+                    // TODO: ideally we'd like other plugins to designate a commit to put the status update to
+                    listener.getLogger().println("Commit doesn't exist in " +


+ on newline to pass checkstyle

michaelneale · 2016-03-07T23:33:50Z

🐝 one checkstyle violation is fixed.

KostyaSha · 2016-03-07T23:54:03Z

no associated jira issue
This changes the strategy of publisher that is totally wrong. Such error can appear because of wrong permissions and then it should fail. If you want special handling, then introduce error handler i.e. like https://github.com/jenkinsci/github-integration-plugin/blob/master/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/utils/PublisherErrorHandler.java
👎

kohsuke · 2016-03-08T00:58:08Z

Note that I'm only catching FileNotFoundException so that it can differentiate other configuration errors from "no such commit" error

KostyaSha · 2016-03-08T01:00:18Z

What error is used when user has no permissions to repository at all?

kohsuke · 2016-03-08T01:00:41Z

Generic IOExcception is used for anything but 404 in github-api

jenkinsadmin · 2016-03-08T01:01:50Z

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

kohsuke · 2016-03-08T01:13:25Z

I created JENKINS-33371 as per your request

KostyaSha · 2016-03-08T09:35:03Z

src/main/java/com/cloudbees/jenkins/GitHubCommitNotifier.java

@@ -52,6 +55,8 @@
    private final String resultOnFailure;
    private static final Result[] SUPPORTED_RESULTS = {FAILURE, UNSTABLE, SUCCESS};

+    private static final Logger LOGGER = Logger.getLogger(GitHubCommitNotifier.class.getName());


use org.slf4j.Logger

lanwen · 2016-03-08T19:06:57Z

think its ok to handle exception with message and not fail the build.
Please remove UnusedImports: Unused import - java.util.logging.Level. and change logger class and ill will merge it

Also please squash dummy commits such as 'sonar, checkstyle fixes' (squashing all be good too)

KostyaSha · 2016-03-08T19:13:42Z

think its ok to handle exception with message and not fail the build.

Not really.

First question would be where notifier extracting commit. If it come from push notification, then it may be not correct and then github-plugin should be fixed at all.
-- Builder can set Pending, then publisher fail and GH will have wrong status.
This is the strategy: fail on errors, skip errors silently. It should be configurable. Either people will PR changes every time or would report bugs for this traces.
Seems Builder forgotten again.

KostyaSha · 2016-03-08T19:44:12Z

First question would be where notifier extracting commit. If it come from push notification, then it may be not correct and then github-plugin should be fixed at all.

Found, that's org.jenkinsci.plugins.github.util.BuildDataHelper#getCommitSHA1 that extracts commit from BuildData. AFAIR BuildData stores sha1 for commit that was defined in checkout section. So any GIT scm features shouldn't change initial commit. AFAIR git SCM design (and BuildData?) has real and effective hashes.

Push trigger fully relies on git scm and git scm should provide correct hashes. Would be glad to hear real example and hash changes situation for git scm.

KostyaSha · 2016-03-08T19:50:36Z

hudson.plugins.git.util.BuildData#buildsByBranchName:

    public Map<String, Build> buildsByBranchName = new HashMap<String, Build>();

-> hudson.plugins.git.util.Build#marked should be right commit, right?

lanwen · 2016-03-08T20:14:00Z

This builder should be splitted to 3 parts - the hash finder, status and message logic and the result handler (of plugin itself), so now build-safe behaviour can be applied

KostyaSha · 2016-03-08T21:05:21Z

@kohsuke please try change BuilDataHelper to use

---        final Revision lastBuildRevision = buildData.getLastBuiltRevision();
+++        final Revision lastBuildRevision = buildData.lastBuild.marked;

KostyaSha · 2016-03-08T21:07:08Z

@lanwen absent commit is gitscm or github plugins bug or force push. Force push of course should fail the build as commit doesn't exist anymore and for example pushing tags after it makes no sense.

kohsuke · 2016-03-08T23:21:26Z

Added what Kostya wanted in another commit and squashed the original one.

KostyaSha · 2016-03-09T11:34:29Z

@kohsuke does that work? Please remove try then as it would be bug in git plugins. GH commit kicks build and it should exist, either build should be failed.

KostyaSha · 2016-03-09T11:37:52Z

cc @oleg-nenashev as BuildDataHelper seems his class

KostyaSha · 2016-03-09T11:42:09Z

If try would be removed back, then 👍 as i think it was initial bug on using wrong commits because BuilData design is to crazy for understanding.

KostyaSha · 2016-03-09T11:42:38Z

src/test/java/com/cloudbees/jenkins/GitHubCommitNotifierTest.java

@@ -73,6 +73,7 @@
        @Override
        protected void before() throws Throwable {
            when(data.getLastBuiltRevision()).thenReturn(rev);
+            data.lastBuild = new hudson.plugins.git.util.Build(rev,rev,0,Result.SUCCESS);


please use imports

and spaces (even if checkstyle doesn't cover test classes).

lanwen · 2016-03-09T14:45:53Z

Please don't forget about https://github.com/jenkinsci/github-plugin/pull/111/files#diff-752cdecf02b12bb22c1fe3285f4d99a5R58

michaelneale · 2016-04-06T22:50:51Z

bump - I guess the remaining changes are to fix the import - is that all?
(whilst this won't fail the build, it won;t correctly set the status of the actual commit what was merge validated).

lanwen · 2016-04-07T11:42:59Z

import + logger. Will fix that on the weekend by myself

michaelneale reviewed Mar 7, 2016
View reviewed changes

michaelneale mentioned this pull request Mar 8, 2016

[UX-108] Enable eslint via js-builder jenkinsci/blueocean-plugin#55

Merged

KostyaSha reviewed Mar 8, 2016
View reviewed changes

kohsuke added 2 commits March 8, 2016 15:08

Don't let the build fail if the commit doesn't exist in upstream

89ac151

Using the 'marked' revision that works better with merge commits

ddef520

kohsuke force-pushed the failed-update branch from d99ee2d to ddef520 Compare March 8, 2016 23:20

KostyaSha reviewed Mar 9, 2016
View reviewed changes

lanwen mentioned this pull request Mar 10, 2016

[JENKINS-33228] Misc global config page improvements #112

Merged

change logger in commit notifier, reformat code in tests for notifiers

e1728e4

lanwen merged commit 25d7939 into master Apr 9, 2016

lanwen deleted the failed-update branch April 9, 2016 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Don't let the build fail if the commit doesn't exist in upstream #111

Don't let the build fail if the commit doesn't exist in upstream #111

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Don't let the build fail if the commit doesn't exist in upstream #111

Don't let the build fail if the commit doesn't exist in upstream #111

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!