Add setters to the StashNotifier.DescriptorImpl class #201

alexchiri · 2018-08-21T14:18:49Z

In order to be able to configure this plugin using the Configuration As Code plugin, the StashNotifier.DescriptorImpl needs to have setters or public fields. Using YAML to configure Jenkins is pretty nice and this would enable this to work with this plugin as well.

scaytrase · 2018-08-21T17:37:18Z

All or most of this properties could be set while construction. While should we allow dynamically change the settings during lifecycle. Isn't it enough to pass YAML configuration to constructor in some way?

alexchiri · 2018-08-22T07:30:49Z

@scaytrase Unfortunately, at least as far as I can see in the plugin implementation, it works by discovering fields and setters to determine what properties can be set from the YAML. They have some guidelines for plugin developers here. There can be some custom implementation be done on the configuration-as-code plugin side to handle different ways of configuring plugins, but I felt since stash notifier has quite a basic config, that would be a bit of an overkill.

darxriggs

Please also use data-binding via request.bindJson(this, json) inside configure(...).
See Step 5 in https://github.com/jenkinsci/configuration-as-code-plugin/blob/configuration-as-code-1.8/docs/PLUGINS.md.

There should also be two tests - one for configuring Jenkins via YAML and one for exporting a configured Jenkins to YAML.
See How to test? in https://github.com/jenkinsci/configuration-as-code-plugin/blob/configuration-as-code-1.8/docs/PLUGINS.md.

Note: I am not a maintainer of this plugin. But I already have migrated some other plugins to be compatible with JCasC.

darxriggs · 2019-03-26T19:44:25Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

@@ -611,34 +611,66 @@ public String getStashRootUrl() {
            }
        }

+        public void setStashRootUrl(String stashRootUrl) {
+			this.stashRootUrl = stashRootUrl;


Please use correct indentation for all new code.
Here and in other places spaces & tabs are currently mixed up.

darxriggs · 2019-03-26T19:45:40Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

@@ -611,34 +611,66 @@ public String getStashRootUrl() {
            }
        }

+        public void setStashRootUrl(String stashRootUrl) {


Please annotate all setters with @DataboundSetter.
See Step 1 in https://github.com/jenkinsci/configuration-as-code-plugin/blob/configuration-as-code-1.8/docs/PLUGINS.md.

alexchiri · 2019-03-27T08:06:54Z

@darxriggs Thank you for the feedback and review. Will work further on the PR soon.

scaytrase · 2019-03-27T09:00:44Z

@alexchiri as soon I have no views on architectural future of this plugin version (only some thoughts on 2.x version), I think we can accept this PR after suggested impovements

…to run the tests in IntelliJIDEA)

… to match JCasC plugin. Add JCasC plugin test dependency to be able to test config as code functionality. Add matrix-project plugin as a test dependency, since it is required in tests by the JenkinsRule. Fix spacing and add @DataBoundSetter annotations to setters. Rewrite configure method and test to use req.bindJSON. Add tests to verify that configure using yaml and export to yaml work.

darxriggs

Thanks for the quick update.
I had another look at the new changes. It already looks pretty good.
See my comments for some required changes.

darxriggs · 2019-03-30T08:35:29Z

pom.xml

        <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>


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.)

darxriggs · 2019-03-30T08:36:12Z

pom.xml

@@ -3,20 +3,20 @@
    <parent>
        <groupId>org.jenkins-ci.plugins</groupId>
        <artifactId>plugin</artifactId>
-        <version>3.10</version>
+        <version>3.36</version>


If you like, there is already 3.40 - see https://github.com/jenkinsci/plugin-pom/releases.

darxriggs · 2019-03-30T08:38:35Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

        public String getProjectKey() {
            return projectKey;
        }

+        public void setProjectKey(String projectKey) {


It seems you missed a @DataBoundSetter here.

darxriggs · 2019-03-30T08:50:15Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

@@ -637,34 +626,73 @@ public String getStashRootUrl() {
            }
        }

+        @DataBoundSetter
+        public void setStashRootUrl(String stashRootUrl) {
+            this.stashRootUrl = stashRootUrl;


For Strings like this one I suggest to use something like Apache Commons Lang StringUtils::trimToNull or Jenkins Util::fixEmptyAndTrim. This improves user experience by better handling strings with leading/trailing spaces.

darxriggs · 2019-03-30T08:50:47Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

        public String getProjectKey() {
            return projectKey;
        }

+        public void setProjectKey(String projectKey) {
+            this.projectKey = projectKey;


See comment about setStashRootUrl() on trimming.

darxriggs · 2019-03-30T09:01:26Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

-            stashRootUrl = formData.getString("stashRootUrl");
-            ignoreUnverifiedSsl = formData.getBoolean("ignoreUnverifiedSsl");
-            includeBuildNumberInKey = formData.getBoolean("includeBuildNumberInKey");
+            this.stashRootUrl = null;


I suggest to keep the order consistent for easier reading/comparison:

field declarations in StashNotifier (maybe)

setters/getters in StashNotifier (maybe)

default values (lines 751-758) in StashNotifier::configure

assertThat calls (lines 27-33) in ConfigAsCodeTest::should_support_jcasc_from_yaml

setter calls (lines 40-47) in ConfigAsCodeTest::should_support_jcasc_to_yaml

darxriggs · 2019-03-30T09:05:29Z

...est/resources/org/jenkinsci/plugins/stashNotifier/ConfigAsCodeTest/configuration-as-code.yml

+    includeBuildNumberInKey: true
+    prependParentProjectKey: true
+    projectKey: "JEN"
+    stashRootUrl: "https://bitbucket.com"


Even if it's only an example value, I suggest to use something like https://my.company.intranet/bitbucket. This more clearly states that this plugin is about "Bitbucket Server" and not "Bitbucket Cloud". Also see #225 for a cleanup regarding this.

darxriggs · 2019-03-30T09:16:22Z

src/test/java/org/jenkinsci/plugins/stashNotifier/DescriptorImplTest.java

-        assertThat(desc.isIncludeBuildNumberInKey(), is(true));
-        assertThat(desc.isPrependParentProjectKey(), is(true));
-        assertThat(desc.isApplicable(AbstractProject.class), is(true));
+        verify(request).bindJSON(desc, json);


The question is, if just verifying a bindJSON call is sufficient for you.
To be really sure, values should still be verified (using a StaplerRequest implementation/mock that implements bindJSON if possible). Otherwise if you for example missed a setter for a field, this would not be caught here and JCasC and UI based configuration would be broken.

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

Update parent version to latest. Remove Maven properties that are inherited anyway from parent pom. Order plugin attributes in the same way everywhere. Trim String attributes. Add forgotten @DataBoundSetter. Update example value for stashRootUrl. Implement more rigorous unit test for configure method.

alexchiri · 2019-03-30T16:23:35Z

Thanks again for the detailed feedback and advice on this @darxriggs. Learned some things and feel I'm getting better at it. 😊

Please let me know if anything else sticks out!

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

darxriggs · 2019-03-30T16:44:23Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

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


You moved the field projectKey before the field stashRootUrl but the setter/getter order doesn't match this. But it doesn't really matter.

darxriggs · 2019-03-30T17:06:03Z

src/test/java/org/jenkinsci/plugins/stashNotifier/DescriptorImplTest.java

@@ -74,20 +77,30 @@ public void testConfigure() throws Descriptor.FormException {
        //given
        doNothing().when(desc).save();

+        ServletContext servletContext = PowerMockito.mock(ServletContext.class);


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.

darxriggs · 2019-03-30T17:24:03Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

        private boolean ignoreUnverifiedSsl;
        private boolean includeBuildNumberInKey;
-        private String projectKey;


@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?

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

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

darxriggs · 2019-03-30T17:44:54Z

Apart from the projectKey field issue, which is to be decided by @scaytrase, this pull request now looks good to me.
I suggest to squash all the commits into one before merging.

scaytrase · 2019-04-02T05:30:49Z

thank you @alexchiri ! (and @darxriggs for comments, for sure)

proski

It was surprising to find this large commit in the commit history with a description that makes it sound like a minor change.

proski · 2019-04-03T17:39:30Z

pom.xml

@@ -143,6 +135,18 @@
            <version>2.0</version>
            <type>jar</type>
        </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>matrix-project</artifactId>


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.

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

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)

Thank you for the explanation!

proski · 2019-04-03T17:41:39Z

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java

-import hudson.FilePath;
-import hudson.Launcher;
-import hudson.ProxyConfiguration;
+import hudson.*;


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

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.

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

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.

darxriggs · 2019-04-20T16:28:20Z

@scaytrase I am looking forward for this to be released. :-)

darxriggs · 2019-04-20T16:33:40Z

Just as reference, there is also JENKINS-56899 tracking this.

Add setters to the StashNotifier.DescriptorImpl class

c9f09c3

darxriggs reviewed Mar 26, 2019

View reviewed changes

alexchiri added 3 commits March 27, 2019 19:47

Merge branch 'release/1.x' into add-setters-to-stash-notifier-descriptor

c328730

Update version of jenkins-test-harness to the latest version (useful …

0f09849

…to run the tests in IntelliJIDEA)

darxriggs requested changes Mar 30, 2019

View reviewed changes

darxriggs reviewed Mar 30, 2019

View reviewed changes

src/main/java/org/jenkinsci/plugins/stashNotifier/StashNotifier.java Show resolved Hide resolved

darxriggs reviewed Mar 30, 2019

View reviewed changes

alexchiri added 2 commits March 30, 2019 17:45

Remove null check on getter.

ff403e0

Update order of getters/setters.

897df5f

darxriggs reviewed Mar 30, 2019

View reviewed changes

Replace PowerMockito with Mockito in unit test.

8206328

scaytrase self-requested a review April 1, 2019 19:25

scaytrase approved these changes Apr 1, 2019

View reviewed changes

scaytrase merged commit 38063fe into jenkinsci:release/1.x Apr 1, 2019

scaytrase mentioned this pull request Apr 1, 2019

Replace all user-facing "Stash" with "Bitbucket Server" #225

Merged

proski reviewed Apr 3, 2019

View reviewed changes

Add setters to the StashNotifier.DescriptorImpl class #201

Add setters to the StashNotifier.DescriptorImpl class #201

Conversation

alexchiri commented Aug 21, 2018

scaytrase commented Aug 21, 2018

alexchiri commented Aug 22, 2018

darxriggs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darxriggs Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

alexchiri commented Mar 27, 2019

scaytrase commented Mar 27, 2019

darxriggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexchiri commented Mar 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darxriggs commented Mar 30, 2019

scaytrase commented Apr 2, 2019 • edited Loading

proski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexchiri Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darxriggs commented Apr 20, 2019 • edited Loading

darxriggs commented Apr 20, 2019

darxriggs left a comment •

edited

Loading

darxriggs Mar 26, 2019 •

edited

Loading

alexchiri commented Mar 30, 2019 •

edited

Loading

scaytrase commented Apr 2, 2019 •

edited

Loading

alexchiri Apr 3, 2019 •

edited

Loading

darxriggs commented Apr 20, 2019 •

edited

Loading