Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-60393] Ensure all plugins are loaded to configure the defaults #136

Merged
merged 7 commits into from Dec 16, 2019
Merged

[JENKINS-60393] Ensure all plugins are loaded to configure the defaults #136

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1949cf3
[JENKINS-60393] Ensure all plugins are loaded to configure the defaults
Dohbedoh Dec 12, 2019
c672384
[code-review] Fix typo
Dohbedoh Dec 13, 2019
4a9a46a
[JENKINS-60393] Add test for the saving of empty defaults
Dohbedoh Dec 13, 2019
b441c87
[JENKINS-60393] Fix the saving of empty defaults
Dohbedoh Dec 13, 2019
8db93bb
[JENKINS-60393] Add a test for component initialization
Dohbedoh Dec 16, 2019
451e2f3
[SQUASH OR FIX] [JENKINS-60393] Revert to show test failure
Dohbedoh Dec 16, 2019
ae00f49
[SQUASH OR FIX] [JENKINS-60393] Apply fix again to show test success
Dohbedoh Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 31 additions & 7 deletions src/main/java/com/cloudbees/hudson/plugins/folder/config/AbstractFolderConfiguration.java
View file
Open in desktop
Expand Up @@ -3,11 +3,15 @@
import com.cloudbees.hudson.plugins.folder.health.FolderHealthMetric;
import com.cloudbees.hudson.plugins.folder.health.FolderHealthMetricDescriptor;
import hudson.Extension;
import hudson.init.InitMilestone;
import hudson.init.Initializer;
import hudson.util.DescribableList;
import jenkins.model.GlobalConfiguration;
import net.sf.json.JSONObject;
import org.jenkinsci.Symbol;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.StaplerRequest;

import javax.annotation.Nonnull;
import java.util.ArrayList;
Expand All @@ -18,7 +22,7 @@
public class AbstractFolderConfiguration extends GlobalConfiguration {

private List<FolderHealthMetric> healthMetrics;

@Nonnull
public static AbstractFolderConfiguration get() {
AbstractFolderConfiguration instance = GlobalConfiguration.all().get(AbstractFolderConfiguration.class);
Expand All @@ -30,27 +34,47 @@ public static AbstractFolderConfiguration get() {

@DataBoundConstructor
public AbstractFolderConfiguration() {
load();
if (healthMetrics == null) {
this.load();
}

/**
* Auto-configure the default metrics after all plugins have been loaded.
*/
@Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.JOB_LOADED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember the implications of having both after and before rules in the same initializer. But I guess you've verified it works as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this yes. I am not sure how to create a test for the initialization though ?
The real fix is to perform the auto configuration outside the constructor and when plugins are loaded: after = InitMilestone.EXTENSIONS_AUGMENTED. I do believe that this is enough. I added the before = InitMilestone.JOB_LOADED because I feel this is consistent (the global metrics will be used on Folder's creation)

public static void autoConfigure() {
AbstractFolderConfiguration abstractFolderConfiguration = AbstractFolderConfiguration.get();
if (abstractFolderConfiguration.healthMetrics == null) {
List<FolderHealthMetric> metrics = new ArrayList<>();
for (FolderHealthMetricDescriptor d : FolderHealthMetricDescriptor.all()) {
FolderHealthMetric metric = d.createDefault();
if (metric != null) {
metrics.add(metric);
}
}
healthMetrics = new DescribableList<FolderHealthMetric, FolderHealthMetricDescriptor>(this, metrics);
abstractFolderConfiguration.setHealthMetrics(new DescribableList<FolderHealthMetric,
FolderHealthMetricDescriptor>(abstractFolderConfiguration, metrics));
}
}

@Nonnull
public List<FolderHealthMetric> getHealthMetrics() {
return healthMetrics == null ? Collections.emptyList() : healthMetrics;
}

@DataBoundSetter
public void setHealthMetrics(List<FolderHealthMetric> healthMetrics) {
this.healthMetrics = healthMetrics;
save();
}
}

@Override
public boolean configure(StaplerRequest req, JSONObject json) {
if(json.containsKey("healthMetrics")) {
req.bindJSON(this, json);
this.save();
} else {
this.setHealthMetrics(Collections.emptyList());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed save() ?

Maybe simpler to

this.setHealthMetrics(Collections.emptyList());
req.bindJSON(this, json);
this.save();

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see now the call to save() in setHealthMetrics

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this would save() twice

return true;
}
}
16 changes: 15 additions & 1 deletion src/test/java/com/cloudbees/hudson/plugins/folder/FolderTest.java
View file
Open in desktop
Expand Up @@ -27,7 +27,6 @@
import com.cloudbees.hudson.plugins.folder.config.AbstractFolderConfiguration;
import com.cloudbees.hudson.plugins.folder.health.FolderHealthMetric;
import com.cloudbees.hudson.plugins.folder.health.FolderHealthMetricDescriptor;
import com.cloudbees.hudson.plugins.folder.health.WorstChildHealthMetric;
import com.cloudbees.hudson.plugins.folder.properties.FolderCredentialsProvider;
import com.cloudbees.plugins.credentials.domains.DomainCredentials;
import com.gargoylesoftware.htmlunit.HttpMethod;
Expand Down Expand Up @@ -431,6 +430,21 @@ private void copyViaHttp(Folder f, JenkinsRule.WebClient wc, String fromName, St
healthMetrics, iterableWithSize(0));
}

@Issue("JENKINS-60393")
@Test public void shouldBeAbleToRemoveHealthMetricConfiguredGlobally() throws Exception {
assertThat("by default, global configuration should have all folder health metrics",
AbstractFolderConfiguration.get().getHealthMetrics(), hasSize(FolderHealthMetricDescriptor.all().size()));

HtmlForm cfg = r.createWebClient().goTo("configure").getFormByName("config");
for (HtmlElement element : cfg.getElementsByAttribute("div", "name", "healthMetrics")) {
element.remove();
}
r.submit(cfg);

assertThat("deleting all global metrics should result in an empty list",
AbstractFolderConfiguration.get().getHealthMetrics(), hasSize(0));
}

/**
* Ensures that the specified property points to the folder.
* @param <T> Property type
Expand Down