[JENKINS-43496] - Add handling of the null Node#createComputer() result. #2922
Conversation
it is a follow-up to jenkinsci#2836 (comment) De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented. In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation.
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
| */ | ||
| @CheckForNull | ||
| @Restricted(NoExternalUse.class) |
There was a problem hiding this comment.
Won't this cause compilation failures for sub-classes? (and if it doesn't then won't the subclass method be invocable anyway, in which case the annotation is useless
There was a problem hiding this comment.
@stephenc @jglick My intention was to reflect the Nobody but {@link Jenkins#updateComputerList()} should call this method. comment in Javadoc. If you know a better annotation for it, just let me know. Not sure if I can dedicate time to writing another restriction if it is missing, so I will just remove it in such case
There was a problem hiding this comment.
@oleg-nenashev ah, were you looking for ProtectedExternally then?
There was a problem hiding this comment.
@jglick Yep, it should work better in this case
| * Nobody but {@link Jenkins#updateComputerList()} should call this method. | ||
| * @return Created instance of the computer. | ||
| * Can be {@code null} if the {@link Node} implementation does not support it (e.g. {@link Cloud} agent). |
There was a problem hiding this comment.
Is this actually true? A Cloud agent would ultimately be a Slave, and that is now documented to return non-null—which seems to contradict your statement that some implementations do return non-null.
There was a problem hiding this comment.
Fixed. Slave#createLauncher() now can create nulls
| @@ -306,6 +306,8 @@ public void setLabelString(String labelString) throws IOException { | |||
| return new GetClockDifference1(); | |||
| } | |||
|
|
|||
| @Override | |||
| @Nonnull | |||
There was a problem hiding this comment.
This is not true in the case of GoogleSlave, from a quick search.
There was a problem hiding this comment.
I agree. Better to just remove since the parent Javadoc describes the null case in a portable way
|
Needs another review |
oleg-nenashev
added
the
ready-for-merge
|
@reviewbybees done, passing to @jenkinsci/code-reviewers |
| used.add(c); | ||
| } else { | ||
| // TODO: Maybe it should be allowed, but we would just get NPE in the original logic before JENKINS-43496 | ||
| LOGGER.log(Level.WARNING, "Node {0} has not executors. Cannot udate the Computer instance for it", n.getNodeName()); |
|
@daniel-beck fine for you? |
|
In IRC @daniel-beck said he sees no obvious issues, hence merging |
…lt. (#2922) * [JENKINS-43496] - Add handling of the null Node#createComputer() result. it is a follow-up to #2836 (comment) De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented. In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation. * [JENKINS-43496] - Use ProtectedExternally to protect Node#createComputer() * [JENKINS-43496] - Remove the erroneous Nonnull annotation after the feedback from @jglick * [JENKINS-43496] - Fix typos noticed by @daniel-beck (cherry picked from commit bcf55ec)
it is a follow-up to #2836 (comment)
De-facto many Cloud plugins return
nullinNode#createLauncher(), but it has never been documented.In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in
AbstractCIBase#updateComputer()which may fail in the case ofnullorRuntimeExceptionin the Node implementation.See JENKINS-43496.
Changelog entries
Proposed changelog entries:
Nodeimplementation in a plugin does not support creation of ad-hoc computers or throws a runtime exception.Submitter checklist
Desired reviewers
@reviewbybees