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

Add setters to the StashNotifier.DescriptorImpl class #201

Merged
merged 8 commits into from Apr 1, 2019
Merged

Add setters to the StashNotifier.DescriptorImpl class #201

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c9f09c3
Add setters to the StashNotifier.DescriptorImpl class
alexchiri Aug 21, 2018
c328730
Merge branch 'release/1.x' into add-setters-to-stash-notifier-descriptor
alexchiri Mar 27, 2019
0f09849
Update version of jenkins-test-harness to the latest version (useful …
alexchiri Mar 30, 2019
61199dd
Update parent plugin version, Jenkins version and Java level version,…
alexchiri Mar 30, 2019
5e62fb0
Make updates based on feedback from @darxriggs
alexchiri Mar 30, 2019
ff403e0
Remove null check on getter.
alexchiri Mar 30, 2019
897df5f
Update order of getters/setters.
alexchiri Mar 30, 2019
8206328
Replace PowerMockito with Mockito in unit test.
alexchiri Mar 30, 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
26 changes: 15 additions & 11 deletions pom.xml
View file
Open in desktop
Expand Up @@ -3,25 +3,17 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>3.10</version>
<version>3.40</version>
<relativePath />
</parent>
<packaging>hpi</packaging>
<properties>
<revision>1.18</revision>
<changelist>-SNAPSHOT</changelist>
<!-- Baseline Jenkins version you use to build the plugin. Users must have this version or newer to run. -->
<jenkins.version>1.625.3</jenkins.version>
<jenkins.version>2.60.3</jenkins.version>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest to reduce it to the following snippet:

<properties>
    <revision>1.18</revision>
    <changelist>-SNAPSHOT</changelist>
    <jenkins.version>2.60.3</jenkins.version>
    <java.level>8</java.level>
</properties>

The rest is already covered by the parent pom nowadays.
(This can be verified with mvn help:effective-pom.)

<!-- Java Level to use. Java 7 required when using core >= 1.612 -->
<java.level>7</java.level>
<!-- Jenkins Test Harness version you use to test the plugin. -->
<!-- For Jenkins version >= 1.580.1 use JTH 2.x or higher. -->
<jenkins-test-harness.version>2.19</jenkins-test-harness.version>
<!-- Other properties you may want to use:
~ hpi-plugin.version: The HPI Maven Plugin version used by the plugin..
~ stapler-plugin.version: The Stapler Maven plugin version required by the plugin.
-->
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<java.level>8</java.level>
</properties>
<artifactId>stashNotifier</artifactId>
<version>${revision}${changelist}</version>
Expand Down Expand Up @@ -143,6 +135,18 @@
<version>2.0</version>
<type>jar</type>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>matrix-project</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was the matrix project dependency added? The commit description makes no mention of it. I don't see the matrix plugin being used in the code. I see that it's only for testing, but I still don't see those new tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a test scope dependency required by the new tests.

Copy link
Contributor Author

@alexchiri alexchiri Apr 3, 2019 •
edited

Choose a reason for hiding this comment

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

Sorry, I missed to add that. I added the Matrix project test dependency because the JenkinsRule requires it and it has been removed from core since 1.561. I added the JenkinsRule in the test that verifies the configuration as code setup, according to JCasC documentation. (import and export yaml). (this is the issue where I found more information about the NoClassDefFoundError I got while using the JenkinsRule: https://issues.jenkins-ci.org/browse/JENKINS-27826)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation!

<version>1.9</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.jenkins</groupId>
<artifactId>configuration-as-code</artifactId>
<version>1.8</version>
<scope>test</scope>
</dependency>
</dependencies>
<build>
<pluginManagement>
Expand Down
118 changes: 67 additions & 51 deletions src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java
View file
Open in desktop
Expand Up @@ -21,10 +21,7 @@
import com.cloudbees.plugins.credentials.common.*;
import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials;
import com.cloudbees.plugins.credentials.domains.DomainRequirement;
import hudson.Extension;
import hudson.FilePath;
import hudson.Launcher;
import hudson.ProxyConfiguration;
import hudson.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think wildcards make the code better. There are plenty of tools to keep an up-to-date list of used imports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me, using wildcard imports or not is a project decision. There are as always advantages and disadvantages. And nowadays tools should be able to handle both.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree on all counts, I just wanted to make sure it was an intentional change and not somebody's sloppiness that everybody missed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not in general a Java developer and I adopted (ocassionaly, huh) this plugin with worse code conventions than it have today, but there is still no CSC strictly defined here today. We can adopt one, I think, if it make life easier.

import hudson.model.*;
import hudson.plugins.git.Revision;
import hudson.plugins.git.util.BuildData;
Expand Down Expand Up @@ -69,10 +66,7 @@
import org.jenkinsci.plugins.displayurlapi.DisplayURLProvider;
import org.jenkinsci.plugins.tokenmacro.MacroEvaluationException;
import org.jenkinsci.plugins.tokenmacro.TokenMacro;
import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.*;

import javax.annotation.Nonnull;
import javax.net.ssl.SSLContext;
Expand Down Expand Up @@ -311,10 +305,6 @@ private boolean perform(Run<?, ?> run,
private String getRootUrl() {
Jenkins instance = Jenkins.getInstance();

if (null == instance) {
scaytrase marked this conversation as resolved.
Show resolved Hide resolved
return globalConfig.getUrl();
}

return (instance.getRootUrl() != null) ? instance.getRootUrl() : globalConfig.getUrl();
}

Expand Down Expand Up @@ -531,10 +521,6 @@ public boolean isTrusted(X509Certificate[] chain, String authType)

private void configureProxy(HttpClientBuilder builder, URL url) {
Jenkins jenkins = Jenkins.getInstance();
if (jenkins == null) {
scaytrase marked this conversation as resolved.
Show resolved Hide resolved
return;
}

ProxyConfiguration proxyConfig = jenkins.proxy;
if (proxyConfig == null) {
return;
Expand Down Expand Up @@ -584,14 +570,14 @@ public static final class DescriptorImpl
* If you don't want fields to be persisted, use <tt>transient</tt>.
*/

private boolean considerUnstableAsSuccess;
private String credentialsId;
private String stashRootUrl;
private boolean disableInprogressNotification;
private boolean ignoreUnverifiedSsl;
private boolean includeBuildNumberInKey;
private String projectKey;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@scaytrase I am wondring why projectKey is a field in StashNotifier.DescriptorImpl. It is not visible in the UI and was never part of global.jelly. Shouldn't it be removed from StashNotifier.DescriptorImpl and only be a field in StashNotifier?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure for it really, may be a copy-paste coincident. We can try to remove it, cannot image when project key should be declared on global level

Copy link
Member

Choose a reason for hiding this comment

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

if it's non blocking for this PR we can try it as a separate change

private boolean prependParentProjectKey;
private boolean disableInprogressNotification;
private boolean considerUnstableAsSuccess;
private String projectKey;
private String stashRootUrl;

public DescriptorImpl() {
this(true);
Expand Down Expand Up @@ -629,42 +615,78 @@ public ListBoxModel doFillCredentialsIdItems(@AncestorInPath Item project) {
return new StandardListBoxModel();
}

public String getStashRootUrl() {
if ((stashRootUrl == null) || (stashRootUrl.trim().isEmpty())) {
return null;
} else {
return stashRootUrl;
}
}

public boolean isDisableInprogressNotification() {
return disableInprogressNotification;
}

public boolean isConsiderUnstableAsSuccess() {
return considerUnstableAsSuccess;
}

@DataBoundSetter
public void setConsiderUnstableAsSuccess(boolean considerUnstableAsSuccess) {
this.considerUnstableAsSuccess = considerUnstableAsSuccess;
}

public String getCredentialsId() {
return credentialsId;
}

@DataBoundSetter
public void setCredentialsId(String credentialsId) {
this.credentialsId = StringUtils.trimToNull(credentialsId);
}

public boolean isDisableInprogressNotification() {
return disableInprogressNotification;
}

@DataBoundSetter
public void setDisableInprogressNotification(boolean disableInprogressNotification) {
this.disableInprogressNotification = disableInprogressNotification;
}

public boolean isIgnoreUnverifiedSsl() {
return ignoreUnverifiedSsl;
}

@DataBoundSetter
public void setIgnoreUnverifiedSsl(boolean ignoreUnverifiedSsl) {
this.ignoreUnverifiedSsl = ignoreUnverifiedSsl;
}

public boolean isIncludeBuildNumberInKey() {
return includeBuildNumberInKey;
}

public String getProjectKey() {
return projectKey;
@DataBoundSetter
public void setIncludeBuildNumberInKey(boolean includeBuildNumberInKey) {
this.includeBuildNumberInKey = includeBuildNumberInKey;
}

public boolean isPrependParentProjectKey() {
return prependParentProjectKey;
}

@DataBoundSetter
public void setPrependParentProjectKey(boolean prependParentProjectKey) {
this.prependParentProjectKey = prependParentProjectKey;
}

public String getProjectKey() {
return projectKey;
}

@DataBoundSetter
public void setProjectKey(String projectKey) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you missed a @DataBoundSetter here.

this.projectKey = StringUtils.trimToNull(projectKey);
}

public String getStashRootUrl() {
scaytrase marked this conversation as resolved.
Show resolved Hide resolved
return stashRootUrl;
}

@DataBoundSetter
public void setStashRootUrl(String stashRootUrl) {
this.stashRootUrl = StringUtils.trimToNull(stashRootUrl);
}

public FormValidation doCheckCredentialsId(@QueryParameter String value, @AncestorInPath Item project)
throws IOException, ServletException {

Expand Down Expand Up @@ -720,26 +742,20 @@ public boolean configure(
StaplerRequest req,
JSONObject formData) throws FormException {

// to persist global configuration information,
// set that to properties and call save().
stashRootUrl = formData.getString("stashRootUrl");
ignoreUnverifiedSsl = formData.getBoolean("ignoreUnverifiedSsl");
includeBuildNumberInKey = formData.getBoolean("includeBuildNumberInKey");
this.considerUnstableAsSuccess = false;
this.credentialsId = null;
this.disableInprogressNotification = false;
this.ignoreUnverifiedSsl = false;
this.includeBuildNumberInKey = false;
this.prependParentProjectKey = false;
this.projectKey = null;
this.stashRootUrl = null;

if (formData.has("credentialsId") && StringUtils.isNotBlank(formData.getString("credentialsId"))) {
credentialsId = formData.getString("credentialsId");
}
if (formData.has("projectKey")) {
projectKey = formData.getString("projectKey");
}
prependParentProjectKey = formData.getBoolean("prependParentProjectKey");

disableInprogressNotification = formData.getBoolean("disableInprogressNotification");

considerUnstableAsSuccess = formData.getBoolean("considerUnstableAsSuccess");
req.bindJSON(this, formData);

save();
return super.configure(req, formData);

return true;
}
}

Expand Down
61 changes: 61 additions & 0 deletions src/test/java/org/jenkinsci/plugins/stashNotifier/ConfigAsCodeTest.java
View file
Open in desktop
@@ -0,0 +1,61 @@
package org.jenkinsci.plugins.stashNotifier;

import hudson.util.IOUtils;
import io.jenkins.plugins.casc.ConfigurationAsCode;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;

import java.io.ByteArrayOutputStream;
import java.io.InputStream;
import java.net.URL;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertThat;

public class ConfigAsCodeTest {
@Rule
public JenkinsRule rule = new JenkinsRule();

@Test
public void should_support_jcasc_from_yaml() throws Exception {
URL configFileUrl = ConfigAsCodeTest.class.getResource(getClass().getSimpleName() + "/configuration-as-code.yml");
ConfigurationAsCode.get().configure(configFileUrl.toString());
StashNotifier.DescriptorImpl stashNotifierConfig = rule.jenkins.getDescriptorByType(StashNotifier.DescriptorImpl.class);

assertThat(stashNotifierConfig.isConsiderUnstableAsSuccess(), equalTo(true));
assertThat(stashNotifierConfig.getCredentialsId(), equalTo("bitbucket-credentials"));
assertThat(stashNotifierConfig.isDisableInprogressNotification(), equalTo(true));
assertThat(stashNotifierConfig.isIgnoreUnverifiedSsl(), equalTo(true));
assertThat(stashNotifierConfig.isIncludeBuildNumberInKey(), equalTo(true));
assertThat(stashNotifierConfig.isPrependParentProjectKey(), equalTo(true));
assertThat(stashNotifierConfig.getProjectKey(), equalTo("JEN"));
assertThat(stashNotifierConfig.getStashRootUrl(), equalTo("https://my.company.intranet/bitbucket"));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Verifying stashRootUrl is missing here.


@Test
public void should_support_jcasc_to_yaml() throws Exception {
StashNotifier.DescriptorImpl stashNotifierConfig = rule.jenkins.getDescriptorByType(StashNotifier.DescriptorImpl.class);

stashNotifierConfig.setConsiderUnstableAsSuccess(true);
stashNotifierConfig.setCredentialsId("bitbucket-credentials");
stashNotifierConfig.setDisableInprogressNotification(true);
stashNotifierConfig.setIgnoreUnverifiedSsl(true);
stashNotifierConfig.setIncludeBuildNumberInKey(true);
stashNotifierConfig.setPrependParentProjectKey(true);
stashNotifierConfig.setProjectKey("JEN");
stashNotifierConfig.setStashRootUrl("https://my.company.intranet/bitbucket");

ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
ConfigurationAsCode.get().export(outputStream);
String exportedYaml = outputStream.toString("UTF-8");

InputStream yamlStream = getClass().getResourceAsStream(getClass().getSimpleName() + "/configuration-as-code.yml");
String expectedYaml = IOUtils.toString(yamlStream, "UTF-8")
.replaceAll("\r\n?", "\n")
.replace("unclassified:\n", "");

assertThat(exportedYaml, containsString(expectedYaml));
}
}
43 changes: 28 additions & 15 deletions src/test/java/org/jenkinsci/plugins/stashNotifier/DescriptorImplTest.java
View file
Open in desktop
Expand Up @@ -14,14 +14,17 @@
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.*;
import org.mockito.Mockito;
import org.powermock.api.mockito.PowerMockito;
import org.powermock.core.classloader.annotations.PrepareForTest;
import org.powermock.modules.junit4.PowerMockRunner;

import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import java.io.IOException;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -38,7 +41,7 @@
*/

@RunWith(PowerMockRunner.class)
@PrepareForTest({StashNotifier.DescriptorImpl.class, CredentialsProvider.class, Jenkins.class})
@PrepareForTest({StashNotifier.DescriptorImpl.class, CredentialsProvider.class, Jenkins.class, TokenList.class})
public class DescriptorImplTest {

/**
Expand All @@ -57,14 +60,14 @@ public void setUp() throws Exception {
when(Jenkins.getInstance()).thenReturn(jenkins);

json = new JSONObject();
json.put("stashRootUrl", "http://stash-root-url");
json.put("credentialsId", "someCredentialsId");
json.put("projectKey", "someProjectKey");
json.put("considerUnstableAsSuccess", "true");
json.put("credentialsId", "bitbucket-credentials");
json.put("disableInprogressNotification", "true");
json.put("ignoreUnverifiedSsl", "true");
json.put("includeBuildNumberInKey", "true");
json.put("prependParentProjectKey", "true");
json.put("disableInprogressNotification", "true");
json.put("considerUnstableAsSuccess", "true");
json.put("projectKey", "JEN");
json.put("stashRootUrl", "https://my.company.intranet/bitbucket");

desc = spy(new StashNotifier.DescriptorImpl(false));
}
Expand All @@ -74,20 +77,30 @@ public void testConfigure() throws Descriptor.FormException {
//given
doNothing().when(desc).save();

ServletContext servletContext = PowerMockito.mock(ServletContext.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible try not to use PowerMockito but only Mockito. You can have a look in the internet why to avoid PowerMock.

PowerMockito is mainly required to mock constructors and static methods. That's often the case for legacy code that was written without testability in mind. At first sight that's not the case here.

WebApp webApp = new WebApp(servletContext);

Stapler stapler = PowerMockito.mock(Stapler.class);
PowerMockito.when(stapler.getWebApp()).thenReturn(webApp);

HttpServletRequest servletRequest = PowerMockito.mock(HttpServletRequest.class);
TokenList tokenList = PowerMockito.mock(TokenList.class);

RequestImpl staplerRequest = new RequestImpl(stapler, servletRequest, new ArrayList<>(), tokenList);

//when
desc.configure(mock(StaplerRequest.class), json);
desc.configure(staplerRequest, json);

//then
assertThat(desc.getStashRootUrl(), is("http://stash-root-url"));
assertThat(desc.getCredentialsId(), is("someCredentialsId"));
assertThat(desc.getProjectKey(), is("someProjectKey"));
assertThat(desc.getDisplayName(), is("Notify Stash Instance"));
assertThat(desc.isApplicable(AbstractProject.class), is(true));
assertThat(desc.getCredentialsId(), is("bitbucket-credentials"));
assertThat(desc.isDisableInprogressNotification(), is(true));
assertThat(desc.isConsiderUnstableAsSuccess(), is(true));
assertThat(desc.isIgnoreUnverifiedSsl(), is(true));
assertThat(desc.getDisplayName(), is("Notify Stash Instance"));
assertThat(desc.isIncludeBuildNumberInKey(), is(true));
assertThat(desc.isIgnoreUnverifiedSsl(), is(true));
assertThat(desc.isPrependParentProjectKey(), is(true));
assertThat(desc.isApplicable(AbstractProject.class), is(true));
assertThat(desc.getProjectKey(), is("JEN"));
assertThat(desc.getStashRootUrl(), is("https://my.company.intranet/bitbucket"));
}

@Test
Expand Down