[JENKINS-48985] - Better diagnostics for Old Data Monitor null safety #3240
Conversation
|
@reviewbybees |
|
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. |
| @@ -116,11 +116,11 @@ public final String getSearchUrl() { | |||
| * Mark this monitor as disabled, to prevent this from showing up in the UI. | |||
| */ | |||
| public void disable(boolean value) throws IOException { | |||
| AbstractCIBase hudson = Jenkins.getInstance(); | |||
| Set<String> set = hudson.disabledAdministrativeMonitors; | |||
| AbstractCIBase jenkins = Jenkins.get(); | |||
There was a problem hiding this comment.
Nope. "disabledAdministrativeMonitors" below is a package-scope method, so you need a class from a proper scope to access it. Didn't want to refactor too much
There was a problem hiding this comment.
OK, though you could make it @Restricted(NoExternalUse.class) public.
Maybe worth a comment to the effect that this is bypassing package access.
| @@ -131,7 +131,7 @@ public void disable(boolean value) throws IOException { | |||
| * he wants to ignore. | |||
| */ | |||
| public boolean isEnabled() { | |||
| return !((AbstractCIBase)Jenkins.getInstance()).disabledAdministrativeMonitors.contains(id); | |||
There was a problem hiding this comment.
Because disabledAdministrativeMonitors is a package-scoped object. It is not visible from the parent class in another package. Weird, but I do not want to refactor it right now
| */ | ||
| @CheckForNull |
There was a problem hiding this comment.
Fine but as above you do not need this method to look up a singleton.
oleg-nenashev
added
the
work-in-progress
oleg-nenashev
removed
the
work-in-progress
|
@jglick ready for re-review |
oleg-nenashev
added
the
ready-for-merge
|
@jenkinsci/core I would like to land it if you are fine with it |
| @@ -116,11 +116,11 @@ public final String getSearchUrl() { | |||
| * Mark this monitor as disabled, to prevent this from showing up in the UI. | |||
| */ | |||
| public void disable(boolean value) throws IOException { | |||
| AbstractCIBase hudson = Jenkins.getInstance(); | |||
| Set<String> set = hudson.disabledAdministrativeMonitors; | |||
| AbstractCIBase jenkins = Jenkins.get(); | |||
There was a problem hiding this comment.
OK, though you could make it @Restricted(NoExternalUse.class) public.
Maybe worth a comment to the effect that this is bypassing package access.
|
@jglick I have created https://issues.jenkins-ci.org/browse/JENKINS-55829 as a follow-up so that somebody can address it. |
See JENKINS-48985. It does not fix the issue at all, but it may help with diagnostics of such cases.
Proposed changelog entries
If required (not sure):
Submitter checklist
* Use the
Internal:prefix if the change has no user-visible impact (API, test frameworks, etc.)Desired reviewers
@mention