Repository: jenkinsci/prometheus-plugin (java)

Issue:
collectDiskUsage option is not persistent
### Jenkins and plugins versions report

<details>
  <summary>Environment</summary>

  <!-- Paste your environment details below -->
  ```text
  Jenkins: 2.375.1
OS: Linux - 3.10.0-957.5.1.el7.x86_64
---
ace-editor:1.1
ant:481.v7b_09e538fcca
antisamy-markup-formatter:155.v795fb_8702324
apache-httpcomponents-client-4-api:4.5.13-138.v4e7d9a_7b_a_e61
authentication-tokens:1.4
authorize-project:1.4.0
badge:1.9.1
blueocean:1.26.0
blueocean-autofavorite:1.2.5
blueocean-bitbucket-pipeline:1.26.0
blueocean-commons:1.26.0
blueocean-config:1.26.0
blueocean-core-js:1.26.0
blueocean-dashboard:1.26.0
blueocean-display-url:2.4.1
blueocean-events:1.26.0
blueocean-git-pipeline:1.26.0
blueocean-github-pipeline:1.26.0
blueocean-i18n:1.26.0
blueocean-jwt:1.26.0
blueocean-personalization:1.26.0
blueocean-pipeline-api-impl:1.26.0
blueocean-pipeline-editor:1.26.0
blueocean-pipeline-scm-api:1.26.0
blueocean-rest:1.26.0
blueocean-rest-impl:1.26.0
blueocean-web:1.26.0
bootstrap4-api:4.6.0-5
bootstrap5-api:5.2.1-3
bouncycastle-api:2.26
branch-api:2.1051.v9985666b_f6cc
build-failure-analyzer:2.4.0
build-monitor-plugin:1.13+build.202205140447
build-name-setter:2.2.0
build-timeout:1.25
buildtriggerbadge:251.vdf6ef853f3f5
caffeine-api:2.9.3-65.v6a_47d0f4d1fe
checks-api:1.8.1
cloudbees-bitbucket-branch-source:791.vb_eea_a_476405b
cloudbees-folder:6.800.v71307ca_b_986b
command-launcher:90.v669d7ccb_7c31
commons-lang3-api:3.12.0-36.vd97de6465d5b_
commons-text-api:1.10.0-27.vb_fa_3896786a_7
conditional-buildstep:1.4.2
credentials:1214.v1de940103927
credentials-binding:523.vd859a_4b_122e6
dark-theme:262.v0202a_4c8fb_6a
dashboard-view:2.466.vdfefd95a_b_f8d
data-tables-api:1.12.1-4
display-url-api:2.3.7
durable-task:503.v57154d18d478
echarts-api:5.4.0-1
email-ext:2.92
external-monitor-job:203.v683c09d993b_9
external-workspace-manager:1.2.3
extra-columns:1.25
favorite:2.4.1
font-awesome-api:6.2.1-1
git:4.14.3
git-client:3.13.1
git-server:99.va_0826a_b_cdfa_d
github:1.36.0
github-api:1.303-400.v35c2d8258028
github-branch-source:1696.v3a_7603564d04
gradle:2.1.1
groovy-postbuild:2.5
handlebars:3.0.8
handy-uri-templates-2-api:2.1.8-22.v77d5b_75e6953
htmlpublisher:1.31
hudson-wsclean-plugin:1.0.8
instance-identity:142.v04572ca_5b_265
ionicons-api:31.v4757b_6987003
jackson2-api:2.14.1-313.v504cdd45c18b
jakarta-activation-api:2.0.1-2
jakarta-mail-api:2.0.1-2
javadoc:226.v71211feb_e7e9
javax-activation-api:1.2.0-5
javax-mail-api:1.6.2-8
jaxb:2.3.7-1
jdk-tool:63.v62d2fd4b_4793
jenkins-design-language:1.26.0
jersey2-api:2.37-1
jira:3.8
jjwt-api:0.11.5-77.v646c772fddb_0
jnr-posix-api:3.1.16-1
job-restrictions:0.8
jobConfigHistory:1191.v168c8c2b_956a
jquery:1.12.4-1
jquery3-api:3.6.1-2
jsch:0.1.55.61.va_e9ee26616e7
junit:1166.va_436e268e972
junit-realtime-test-reporter:90.v7dc2c937f9a_8
ldap:2.12
lockable-resources:1069.v726298f53f8c
mailer:438.v02c7f0a_12fa_4
matrix-auth:3.1.6
matrix-project:785.v06b_7f47b_c631
maven-plugin:3.20
metrics:4.2.13-420.vea_2f17932dd6
mina-sshd-api-common:2.9.2-50.va_0e1f42659a_a
mina-sshd-api-core:2.9.2-50.va_0e1f42659a_a
momentjs:1.1.1
okhttp-api:4.9.3-108.v0feda04578cf
p4:1.13.0
pam-auth:1.10
parameterized-trigger:2.45
pipeline-build-step:2.18
pipeline-github-lib:38.v445716ea_edda_
pipeline-graph-analysis:195.v5812d95a_a_2f9
pipeline-groovy-lib:621.vb_44ce045b_582
pipeline-input-step:466.v6d0a_5df34f81
pipeline-milestone-step:101.vd572fef9d926
pipeline-model-api:2.2118.v31fd5b_9944b_5
pipeline-model-declarative-agent:1.1.1
pipeline-model-definition:2.2118.v31fd5b_9944b_5
pipeline-model-extensions:2.2118.v31fd5b_9944b_5
pipeline-rest-api:2.28
pipeline-stage-step:296.v5f6908f017a_5
pipeline-stage-tags-metadata:2.2118.v31fd5b_9944b_5
pipeline-stage-view:2.28
pipeline-utility-steps:2.14.0
plain-credentials:139.ved2b_9cf7587b
plot:2.1.12
plugin-util-api:2.20.0
popper-api:1.16.1-3
popper2-api:2.11.6-2
prometheus:2.0.11
publish-over:0.22
publish-over-ssh:1.24
pubsub-light:1.17
resource-disposer:0.20
role-strategy:569.v7476f8e4fe29
run-condition:1.5
scm-api:631.v9143df5b_e4a_a
script-security:1228.vd93135a_2fb_25
simple-theme-plugin:136.v23a_15f86c53d
snakeyaml-api:1.33-90.v80dcb_3814d35
sse-gateway:1.26
ssh-credentials:305.v8f4381501156
ssh-slaves:2.854.v7fd446b_337c9
sshd:3.275.v9e17c10f2571
structs:324.va_f5d6774f3a_d
test-results-analyzer:0.4.0
theme-manager:1.6
timestamper:1.21
token-macro:321.vd7cc1f2a_52c8
trilead-api:2.84.v72119de229b_7
uno-choice:2.6.4
variant:59.vf075fe829ccb
view-job-filters:2.3
windows-slaves:1.8.1
workflow-aggregator:590.v6a_d052e5a_a_b_5
workflow-api:1200.v8005c684b_a_c6
workflow-basic-steps:994.vd57e3ca_46d24
workflow-cps:3583.v4f58de0d78d5
workflow-cps-global-lib:609.vd95673f149b_b
workflow-durable-task-step:1217.v38306d8fa_b_5c
workflow-job:1254.v3f64639b_11dd
workflow-multibranch:716.vc692a_e52371b_
workflow-scm-step:400.v6b_89a_1317c9a_
workflow-step-api:639.v6eca_cd8c04a_a_
workflow-support:839.v35e2736cfd5c
ws-cleanup:0.44
xvfb:1.2
xvnc:1.28
  ```

</details>


### What Operating System are you using (both controller, and any agents involved in the problem)?

RHEL 7

### Reproduction steps

1. Uncheck collectDiskUsage option in the Jenkins Configuration page and save
2. Restart instance
3. Go to the configuration page, collectDiskUsage is checked again.

### Expected Results

collectDiskUsage option should be persistent and disabled

### Actual Results

collectDiskUsage is enabled again after restart

### Anything else?

When configuration is saved and option is unchecked - everything works fine, but after restart, inside PrometheusConfiguration(), after load(), setCollectDiskUsage(null) is called. This is causing the default value to be taken (true). I know there is an environment variable called COLLECT_DISK_USAGE but this is something hard to maintain while deploying new Jenkins instance. Not even saying that lack of persistence is misleading to the users.

Pull request description:
Bugfix - Collect Disk Usage is stored correctly
Fixes #482

### Changes proposed

- The Environment variable COLLECT_DISK_USAGE takes the lead. If it's set to a valid boolean parameter this configuration wins. Otherwise the configured value over the Jenkins configuration page is taken. If the Environment variable is set, then the configuration item in the Jenkins page is disabled. 

### Checklist

- [x] Ready for review

### Notify

@markyjackson-taulia. Do you think that is a valid fix?


Patch:
diff --git a/src/main/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration.java b/src/main/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration.java
index d876088..4e5b456 100644
--- a/src/main/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration.java
+++ b/src/main/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration.java
@@ -32,8 +32,7 @@ public class PrometheusConfiguration extends GlobalConfiguration {
     private static final String DEFAULT_ENDPOINT = "prometheus";
     static final String COLLECTING_METRICS_PERIOD_IN_SECONDS = "COLLECTING_METRICS_PERIOD_IN_SECONDS";
     static final long DEFAULT_COLLECTING_METRICS_PERIOD_IN_SECONDS = TimeUnit.MINUTES.toSeconds(2);
-    private static final String COLLECT_DISK_USAGE = "COLLECT_DISK_USAGE";
-    static final boolean DEFAULT_COLLECT_DISK_USAGE = true;
+    static final String COLLECT_DISK_USAGE = "COLLECT_DISK_USAGE";
 
     private String urlName = null;
     private String additionalPath;
@@ -55,6 +54,7 @@ public class PrometheusConfiguration extends GlobalConfiguration {
     private boolean appendStatusLabel = false;
     private boolean perBuildMetrics = false;
 
+    private boolean environmentVariableSet;
 
     private String labeledBuildParameterNames = "";
 
@@ -66,7 +66,8 @@ public class PrometheusConfiguration extends GlobalConfiguration {
         load();
         setPath(getPath());
         setCollectingMetricsPeriodInSeconds(collectingMetricsPeriodInSeconds);
-        setCollectDiskUsage(null);
+        setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        environmentVariableSet = isValidBooleanEnv(COLLECT_DISK_USAGE);
     }
 
     public static PrometheusConfiguration get() {
@@ -134,15 +135,33 @@ public class PrometheusConfiguration extends GlobalConfiguration {
     }
 
     public void setCollectDiskUsage(Boolean collectDiskUsage) {
-        if (collectDiskUsage == null) {
-            String value = System.getenv(COLLECT_DISK_USAGE);
-            this.collectDiskUsage = value != null ? Boolean.parseBoolean(value) : DEFAULT_COLLECT_DISK_USAGE;
-        } else {
-            this.collectDiskUsage = collectDiskUsage;
-        }
+        this.collectDiskUsage = collectDiskUsage;
         save();
     }
 
+    public void setCollectDiskUsageBasedOnEnvironmentVariableIfDefined() {
+        try {
+            this.collectDiskUsage = getBooleanEnvironmentVariableOrThrowException(COLLECT_DISK_USAGE);
+        } catch (IllegalArgumentException e) {
+            logger.warn("Unable to parse environment variable '{}'. Must either be 'true' or 'false'. Ignoring...", COLLECT_DISK_USAGE);
+        }
+    }
+
+    private boolean getBooleanEnvironmentVariableOrThrowException(String key) throws IllegalArgumentException {
+        if (isValidBooleanEnv(key)) {
+            return Boolean.parseBoolean(System.getenv(key));
+        }
+        throw new IllegalArgumentException();
+    }
+
+    private boolean isValidBooleanEnv(String key) {
+        String value = System.getenv(key);
+        if (value != null) {
+            return ("true".equalsIgnoreCase(value) || "false".equalsIgnoreCase(value));
+        }
+        return false;
+    }
+
     public boolean getCollectDiskUsage() {
         return collectDiskUsage;
     }
@@ -328,4 +347,8 @@ public class PrometheusConfiguration extends GlobalConfiguration {
         }
         return stringValue.split("\\s*,\\s*");
     }
+
+    public boolean isEnvironmentVariableSet() {
+        return environmentVariableSet;
+    }
 }
diff --git a/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/config.jelly b/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/config.jelly
index 4b29a7f..9656cb8 100644
--- a/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/config.jelly
+++ b/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/config.jelly
@@ -47,7 +47,8 @@
       <f:textbox />
     </f:entry>
     <f:entry title="${%Collect disk usage}" field="collectDiskUsage">
-      <f:checkbox/>
+      <j:set var="readOnlyMode" value="${instance.isEnvironmentVariableSet()}"/>
+      <f:checkbox />
     </f:entry>
     <f:entry title="${%Collect node status}" field="collectNodeStatus">
       <f:checkbox/>
diff --git a/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/help-collectDiskUsage.jelly b/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/help-collectDiskUsage.jelly
index ab0c4f3..85a0855 100644
--- a/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/help-collectDiskUsage.jelly
+++ b/src/main/resources/org/jenkinsci/plugins/prometheus/config/PrometheusConfiguration/help-collectDiskUsage.jelly
@@ -3,6 +3,8 @@
   <div>
     <p>
       If prometheus should collect disk usage or not. Disable this if you are using a cloud storage provider.
+      You can overwrite the default behavior with the environment variable COLLECT_DISK_USAGE.
+      This field is disabled if the variable is set.
     </p>
   </div>
 </j:jelly>


Test patch:
diff --git a/src/test/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfigurationTest.java b/src/test/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfigurationTest.java
index 711b690..7067b70 100644
--- a/src/test/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfigurationTest.java
+++ b/src/test/java/org/jenkinsci/plugins/prometheus/config/PrometheusConfigurationTest.java
@@ -99,22 +99,6 @@ public class PrometheusConfigurationTest {
         assertThat(actual).isEqualTo(1000);
     }
 
-    @Test
-    public void shouldSetValueFromEnvForCollectDiskUsage() throws Exception{
-        // given
-        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsage(any());
-        Mockito.when(configuration.getCollectDiskUsage()).thenCallRealMethod();
-        Boolean collectDiskUsage = null;
-
-        // when
-        withEnvironmentVariable(String.valueOf(PrometheusConfiguration.DEFAULT_COLLECT_DISK_USAGE), String.valueOf(true))
-                .execute(() -> configuration.setCollectDiskUsage(collectDiskUsage));
-        boolean actual = configuration.getCollectDiskUsage();
-
-        // then
-        assertThat(actual).isEqualTo(true);
-    }
-
     @Test
     @Parameters(method = "wrongMetricCollectorPeriodsProvider")
     public void shouldSetDefaultValueWhenEnvCannotBeConvertedToLongORNegativeValue(String wrongValue) throws Exception {
@@ -132,6 +116,87 @@ public class PrometheusConfigurationTest {
         assertThat(actual).isEqualTo(PrometheusConfiguration.DEFAULT_COLLECTING_METRICS_PERIOD_IN_SECONDS);
     }
 
+    @Test
+    public void shouldTakeDefaultValueWhenNothingConfigured() {
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        Mockito.doCallRealMethod().when(configuration).getCollectDiskUsage();
+
+        // simulate constructor call
+        configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+
+
+        assertThat(configuration.getCollectDiskUsage()).isFalse();
+    }
+
+    @Test
+    public void shouldTakeEnvironmentVariableWhenNothingConfigured() throws Exception {
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        Mockito.doCallRealMethod().when(configuration).getCollectDiskUsage();
+
+        withEnvironmentVariable(PrometheusConfiguration.COLLECT_DISK_USAGE, "false")
+                .execute(() -> {
+                    // simulate constructor call
+                    configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+                });
+
+        assertThat(configuration.getCollectDiskUsage()).isFalse();
+    }
+
+    @Test
+    public void shouldTakeDefaultIfEnvironmentVariableIsFaulty() throws Exception {
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        Mockito.doCallRealMethod().when(configuration).getCollectDiskUsage();
+
+        withEnvironmentVariable(PrometheusConfiguration.COLLECT_DISK_USAGE, "not_true_not_false")
+                .execute(() -> {
+                    // simulate constructor call
+                    configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+                });
+
+        assertThat(configuration.getCollectDiskUsage()).isFalse();
+    }
+
+    @Test
+    public void shouldTakeConfiguredValueIfEnvironmentVariableIsFaulty() throws Exception {
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        Mockito.doCallRealMethod().when(configuration).getCollectDiskUsage();
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsage(any());
+
+        withEnvironmentVariable(PrometheusConfiguration.COLLECT_DISK_USAGE, "not_true_not_false")
+                .execute(() -> {
+
+                    // simulate user clicked on checkbox
+                    configuration.setCollectDiskUsage(true);
+
+                    // simulate constructor call
+                    configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+                });
+
+        assertThat(configuration.getCollectDiskUsage()).isTrue();
+    }
+
+    @Test
+    public void shouldTakeConfiguredValueIfItIsConfigured() {
+        Mockito.doCallRealMethod().when(configuration).setCollectDiskUsage(any());
+        Mockito.doCallRealMethod().when(configuration).getCollectDiskUsage();
+
+        // simulate someone configured it over the UI
+        configuration.setCollectDiskUsage(false);
+        assertThat(configuration.getCollectDiskUsage()).isFalse();
+
+        // simulate constructor call
+        configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        assertThat(configuration.getCollectDiskUsage()).isFalse();
+
+        // simulate someone configured it over the UI
+        configuration.setCollectDiskUsage(true);
+        assertThat(configuration.getCollectDiskUsage()).isTrue();
+
+        // simulate constructor call
+        configuration.setCollectDiskUsageBasedOnEnvironmentVariableIfDefined();
+        assertThat(configuration.getCollectDiskUsage()).isTrue();
+    }
+
     private JSONObject getDefaultConfig() {
         JSONObject config = new JSONObject();
         config.accumulate("path", "prometheus");