Refactor global configuration#446
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
nikita-tkachenko-datadog
added
the
changelog/Fixed
9efa308 to
94211bd
Compare
5f3a210 to
d20b98e
Compare
d20b98e to
b8e26e6
Compare
b8e26e6 to
0404aa5
Compare
0404aa5 to
0260867
Compare
drodriguezhdez
left a comment
drodriguezhdez
left a comment
There was a problem hiding this comment.
First pass done. Nice refactor! 🚀
I've dropped some comments (especially important the one related to ensure backwards compatibility when downgrading Jenkins plugin version)
| return CredentialsMatchers.firstOrNull( | ||
| CredentialsProvider.lookupCredentials( | ||
| StringCredentials.class, | ||
| Jenkins.get(), | ||
| ACL.SYSTEM, | ||
| URIRequirementBuilder.fromUri(null).build()), | ||
| CredentialsMatchers.allOf(CredentialsMatchers.withId(credentialId)) | ||
| ); |
There was a problem hiding this comment.
Do we know if any of these methods can throw a runtime exception? (I was inspecting the code and it does not seem to be the case but 🤷🏻)
There was a problem hiding this comment.
I couldn't find any. Anyway, I don't think that suppressing the error is better than propagating it in this case. If credentials cannot be retrieved, the error will eventually be propagated to DatadogGlobalConfiguration#configure where it will be logged and updating the config will be aborted.
| public abstract int getOrder(); | ||
| } | ||
|
|
||
| public static DatadogIntake getDefaultIntake() { |
There was a problem hiding this comment.
Why everything has the prefix default. Is it because this one is the fallback but you can have another non-default instances? Am I missing something?
If only default instances are available, should we remove the prefix default?
There was a problem hiding this comment.
These are the values that are used when corresponding property is not configured in the UI.
These methods are mostly to be called from the jelly files, e.g.
varName="apiKey" instance="${instance.apiKey != null ? instance.apiKey : descriptor.defaultApiKey}"
| public class MetricsTest { | ||
|
|
||
| @Test | ||
| public void testIncrementCountAndFlush() { |
There was a problem hiding this comment.
Is this test is unrelated to the config refactor? Am I missing something?
There was a problem hiding this comment.
This test used to live in DatadogClientTest.java. Most of the test cases there became obsolete after the refactoring, this was the only one that was left. I noticed that it had nothing to do with the client and moved it to a place that seemed more appropriate.
| protected XmlFile getConfigFile() { | ||
| return new XmlFile(XSTREAM, new File(Jenkins.get().getRootDir(), getId() + ".xml")); |
There was a problem hiding this comment.
I think we should have a mechanism to avoid breaking the Jenkins config for previous versions:
- User updates to the new Jenkins plugin version with the config refactor.
- User modifies config and saves the new config. The file format changes.
- User tries to downgrade Jenkins plugin to previous version. Cannot start as the file format is unexpected.
A possible approach could be:
- Store config with a new name (
globalConfig_v2.xml) or something like that - Try to load v2 config first. If it's not found, try to load legacy config.
With that approach, old Jenkins plugin version should still working.
However, not sure about the Config-as-Code format 🤔
There was a problem hiding this comment.
That's a great idea! I implemented the _v2 storing/loading.
As for CasC, the config reads from it but does not write, meaning YAMLs will remain the same unless users change them manually (the new code is compatible with the old YAMLs). So it's not that big a problem as the one with XMLs.
3 participants
Requirements for Contributing to this repository
What does this PR do?
Refactors global plugin configuration. The following is changed:
DatadogGlobalConfigurationwas refactored: it used to contain a flat list of all config properties for different modes (agentless and agentful), now it contains an instance ofDatadogClientConfigurationwhich encapsulates config for the chosen mode (there are different implementations for agentless and agentful modes). The logic for instantiating the correctDatadogClientis also hidden inDatadogClientConfigurationDescription of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional 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.