Skip to content
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

[JENKINS-29470] - Prevent NPE in AbstractProject.checkout when agent disconnects during the build #3094

Merged
merged 4 commits into from Mar 4, 2018

Conversation

@piushkumar
Copy link
Contributor

commented Oct 17, 2017

See JENKINS-29470.

Proposed changelog entries

  • Bug: Prevent NPE in AbstractProject.checkout when agent disconnects during the build
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

workspace.mkdirs();
}
else{
return true;

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 17, 2017

Member

Sounds like an error suppression. Could you please explain this change a bit?

This comment has been minimized.

Copy link
@piushkumar

piushkumar Oct 17, 2017

Author Contributor

Don't think the error will be suppressed. Should be caught in AbstracBuild during onCheckout. Here just avoiding a NPE for null workspace. Or if dont want to suppress it, we can throw a runtime excpetion with proper message if you suggest.

@oleg-nenashev
Copy link
Member

left a comment

According to

if (project.checkout(build, launcher,listener, changeLogFile)) {
// check out succeeded
SCM scm = project.getScm();
for (SCMListener l : SCMListener.all()) {
, returning true means that the checkout succeeded. I do not see how it would be compliant with the case when the workspace is not available for the build.

If the method returns false, I am fine with the current approach (bonus points for logging). If you feel that true has to be returned, the behavior needs to be well documented in Javadoc and covered by tests.

workspace.mkdirs();
}
else{
LOGGER.log(Level.SEVERE,"Workspace does not exist!!");

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 21, 2017

Member

This log message is not particularly useful.

  • Never use multiple exclamation points, or I shall be forced to quote Terry Pratchett 😉 In fact, it's fine to use none at all.
  • Which agent? (build.getBuiltOn() perhaps?)
    • Perhaps it would be useful to include the status of the agent, if safely feasible?
  • Which project?
  • In fact, it's even a misleading statement: The workspace is perhaps undefined, as Jenkins creates it if defined a few lines up. So the problem isn't existence (on the file system).
  • It's not a severe error of the system. WARNING should be fine.

With the current lack of information, the log message is pretty much useless.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 21, 2017

Member

The log message should also go to BuildListener listener IMHO. In such case there will be no need to specify the build.

This comment has been minimized.

Copy link
@jglick

jglick Oct 23, 2017

Member

No logging is needed at all. Just throw an AbortException.

workspace.mkdirs();
}
else{
LOGGER.log(Level.SEVERE,"Workspace does not exist!!");

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 21, 2017

Member

The log message should also go to BuildListener listener IMHO. In such case there will be no need to specify the build.

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Oct 22, 2017

How does this change interact with #3090?

workspace.mkdirs();
}
else{
LOGGER.log(Level.SEVERE,"Workspace does not exist!!");

This comment has been minimized.

Copy link
@jglick

jglick Oct 23, 2017

Member

No logging is needed at all. Just throw an AbortException.

@piushkumar

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2017

@jglick Should it be just an Abort Exception with "workspace is undefined" message ?

workspace.mkdirs();
}
else{
throw new AbortException(); // workspace is undefined

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 27, 2017

Member

The reason should be passed in the exception message. Otherwise the user will not see the root cause in the build log

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 21, 2018

@piushkumar any plans to finish it?

@oleg-nenashev oleg-nenashev self-assigned this Feb 23, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

@jenkinsci/code-reviewers @jglick @olivergondza Since @piushkumar was unresponsive, I went forward and fixed the left bits of this pull requests. Please review

@jglick's comment has been addressed

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

Test failure is clearly unrelated

@oleg-nenashev oleg-nenashev changed the title JENKINS-29470 : NPE in AbstractProject.checkout [JENKINS-29470] - NPE in AbstractProject.checkout Mar 4, 2018

@oleg-nenashev oleg-nenashev changed the title [JENKINS-29470] - NPE in AbstractProject.checkout [JENKINS-29470] - Prevent NPE in AbstractProject.checkout when agent disconnects during the build Mar 4, 2018

@oleg-nenashev oleg-nenashev merged commit b0c5a86 into jenkinsci:master Mar 4, 2018

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details

olivergondza added a commit that referenced this pull request Mar 23, 2018

[JENKINS-29470] - Prevent NPE in AbstractProject.checkout when agent …
…disconnects during the build (#3094)

[JENKINS-29470] - Prevent NPE in AbstractProject.checkout when agent disconnects during the build

(cherry picked from commit b0c5a86)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.