Update config access through lookupSingleton method #85
Conversation
src/test/java/org/datadog/jenkins/plugins/datadog/clients/DatadogClientTest.java
Show resolved
Hide resolved
| return ExtensionList.lookup(DatadogGlobalConfiguration.class).get(DatadogGlobalConfiguration.class); | ||
| } catch(NullPointerException e){ | ||
| return ExtensionList.lookupSingleton(DatadogGlobalConfiguration.class); | ||
| } catch (IllegalStateException e) { | ||
| // It can only throw a NullPointerException when running tests |
There was a problem hiding this comment.
Can you remove/update this comment?
| @@ -646,10 +646,4 @@ private String getJenkinsVersion(){ | |||
| } | |||
| return this.jenkinsVersion; | |||
| } | |||
|
|
|||
| private static boolean isCollectBuildLogEnabled(){ | |||
| return DatadogUtilities.getDatadogGlobalDescriptor() != null && | |||
There was a problem hiding this comment.
So we're not checking for null anymore which is the reason why you had to add https://github.com/jenkinsci/datadog-plugin/pull/85/files#diff-16902ca2fc3a4b1fa3f87d6ad966d842R43 in the tests.
I'm not totally confident in the statement "It can only throw a IllegalStateException when running tests" here. If the DatadogGlobalConfiguration extension fails to load for any reason, the singleton will be null.
There was a problem hiding this comment.
We aren't checking for null as part of the runtime code, correct. It shouldn't be necessary to guard against that since protecting from a failed config load will just hide other problems.
DatadogUtilities do a lot of checking for null which we should actually remove too, but there is deeper logic in our tests that currently depend on the default responses which makes it a more involved change.
The general approaches of other plugins seem to have this behavior as well - see for instance this line from the referenced Azure Keyvault repo: https://github.com/jenkinsci/azure-keyvault-plugin/blob/ef3a5eb4413f7ecd47d6532735604c52fceaaef0/src/main/java/org/jenkinsci/plugins/azurekeyvaultplugin/AzureKeyVaultBuildWrapper.java#L136
sarah-witt
added
the
changelog/Fixed
What does this PR do?
Implements more unified access to global configuration through the single utility function.
What inspired you to submit this pull request?
#59 (comment)
Description of the Change
All configuration requests now go through the utility class, which performs the standard approach of
lookupSingletonfor retrieving the class.This has the drawback of requiring a Jenkins instance during tests (because of the
DatadogGlobalConfiguration.load()operation), else an exception gets raised, so we still wrap the access in a try/catch to maintain support for existing test scenarios.Alternate Designs
Adding a
getmethod on theDatadogGlobalConfigurationclass was the first approach, but in order to maintain support for the existing test suite, this required keeping the existing utility method as well, thus creating a confusing interface. It also made the accesses more verbose versusDatadogUtilities.getDatadogGlobalDescriptor().Possible Drawbacks
Maintaining the try/catch and returning null in some scenarios can hide some potential errors since the other utility methods that check for null return default values which may not be expected and would be hard to troubleshoot.
Verification Process
mvn testAdditional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/label attached. If applicable it should have thebackward-incompatiblelabel attached.do-not-merge/label attached.kind/andseverity/labels attached at least.