-
Notifications
You must be signed in to change notification settings - Fork 395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't let the build fail if the commit doesn't exist in upstream #111
Conversation
// 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 " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+
on newline to pass checkstyle
🐝 one checkstyle violation is fixed. |
|
Note that I'm only catching |
What error is used when user has no permissions to repository at all? |
Generic |
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
I created JENKINS-33371 as per your request |
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use org.slf4j.Logger
think its ok to handle exception with message and not fail the build. Also please squash dummy commits such as 'sonar, checkstyle fixes' (squashing all be good too) |
Not really.
|
Found, that's 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. |
-> |
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 |
@kohsuke please try change BuilDataHelper to use
|
@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. |
Added what Kostya wanted in another commit and squashed the original one. |
@kohsuke does that work? Please remove |
cc @oleg-nenashev as BuildDataHelper seems his class |
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. |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and spaces (even if checkstyle doesn't cover test classes).
Please don't forget about https://github.com/jenkinsci/github-plugin/pull/111/files#diff-752cdecf02b12bb22c1fe3285f4d99a5R58 |
bump - I guess the remaining changes are to fix the import - is that all? |
import + logger. Will fix that on the weekend by myself |
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: