Convert Groovy views to Jelly | use Jenkins.get() #33

Wadeck · 2018-10-28T14:23:54Z

convert groovy view to jelly
use Jenkins.get()
~~reformat code to follow Java convention~~
~~reorder pom file parts to follow convention~~
~~rename test variables to follow Jenkins standard~~

@cloudbees/team-foundation @reviewbybees

jvz · 2018-10-29T15:34:14Z

src/main/resources/hudson/tasks/AntWrapper/config.jelly

+<!--
+The MIT License
+
+Copyright (c) 2016, CloudBees, Inc.


Suggested change

Copyright (c) 2016, CloudBees, Inc.

Copyright (c) 2018, CloudBees, Inc.

Or maybe add 2018 to the list?

batmat

👍

batmat · 2018-10-28T16:19:54Z

pom.xml

-    </repository>
-  </repositories>
+    <properties>
+        <jenkins.version>2.107.3</jenkins.version>


Just a note, this bumps the new min baseline pretty aggressively, but fine by me. Anyway old instances never update or close to it.

batmat · 2018-10-29T20:57:15Z

src/main/java/hudson/tasks/AntWrapper.java

@@ -79,10 +80,10 @@ public void setJdk(String jdk) {
    @Override
    public void setUp(Context context, Run<?, ?> build, FilePath workspace, Launcher launcher, TaskListener listener, EnvVars initialEnvironment) throws IOException, InterruptedException {
        if (installation != null) {
-            toolEnv(context, installation, Jenkins.getActiveInstance().getDescriptorByType(Ant.DescriptorImpl.class).getInstallations(), workspace, listener, initialEnvironment);
+            toolEnv(context, installation, Jenkins.get().getDescriptorByType(Ant.DescriptorImpl.class).getInstallations(), workspace, listener, initialEnvironment);


src/main/resources/hudson/tasks/AntWrapper/config.jelly

batmat · 2018-10-29T21:02:45Z

src/main/resources/hudson/tasks/_ant/AntTargetNote/DescriptorImpl/outline.jelly

+        </table>
+    </l:ajax>
+</j:jelly>


/me not convinced about this kind of change: IMO though tempting, we should refrein from reformating unrelated code to make code archeology easier.

jglick

Hard to see what is actually being changed here and why. Anyway #32 probably covers most of what this was doing.

jglick · 2018-10-29T22:27:40Z

pom.xml

-    <relativePath />
-  </parent>
+<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">


Please do not reformat existing lines of code.

jglick · 2018-10-29T22:27:53Z

src/main/java/hudson/tasks/Ant.java

@@ -1,18 +1,18 @@
 /*
 * The MIT License
- * 
+ *


- convert groovy view to jelly - reformat code to follow Java convention - reorder pom file to follow convention https://maven.apache.org/developers/conventions/code.html - rename test variables to follow Jenkins standard - use Jenkins.get()

Wadeck · 2018-10-30T10:06:05Z

I rebase the PR after the merge of #32. Also remove some of the pure tab/space reformat to keep mostly the relevant one that increase readability.

jglick

Prefer all formatting changes to be reverted. Use of nondeprecated methods, Groovy → Jelly switch, etc. are actual code changes which sound right, if I could find them.

jglick · 2018-10-30T14:14:09Z

src/test/java/hudson/tasks/_ant/AntTargetAnnotationTest.java

@@ -23,7 +23,8 @@
 public class AntTargetAnnotationTest {

    @Rule
-    public JenkinsRule r = new JenkinsRule();


I have actually been preferring the name r, since j is too easily confused with the Jenkins instance itself.

My concern here was essentially because the same name is used for RestartableJenkinsRule r. No worries I revert them.

My style (FWIW!) is to use RestartableJenkinsRule rr.

jglick · 2018-10-30T14:14:29Z

src/test/java/hudson/tasks/_ant/AntTargetNoteTest.java

@@ -5,7 +5,7 @@
 import org.junit.Before;
 import org.junit.Test;

-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;


Common style for static imports in tests is to use wildcards.

jglick · 2018-10-30T14:15:53Z

src/main/java/hudson/tasks/Ant.java

@@ -104,7 +104,7 @@
    private final String properties;

    @DataBoundConstructor
-    public Ant(String targets,String antName, String antOpts, String buildFile, String properties) {
+    public Ant(String targets, String antName, String antOpts, String buildFile, String properties) {


Unfortunately https://github.com/jenkinsci/ant-plugin/pull/33/files?w still does not ignore this, making this diff too long to be read.

jglick · 2018-10-30T14:17:12Z

Anyway I am done “maintaining” this—just needed to deliver #32 urgently.

less reformatting than before

- keep convert groovy view to jelly - keep use Jenkins.get()

MRamonLeon

🐝

fcojfernandez

Re-Approved

jglick

Thanks for your patience!

jglick · 2018-10-31T13:34:30Z

src/main/resources/hudson/tasks/Ant/config.jelly

+-->
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+    <j:if test="${size(descriptor.installations) != 0}">


You tested this logic interactively, right? (Cannot recall if there is a functional test for it.)

(There are actually good reasons to discards the if block and show this dropdown unconditionally, but that would be out of scope for this PR.)

You tested this logic interactively, right?

I tested with and without installations but not sure to have cover all other potential cases there.

jglick · 2018-10-31T13:34:51Z

src/main/resources/hudson/tasks/Ant/config.jelly

+            <select class="setting-input" name="ant.antName">
+                <option value="(Default)">${%Default}</option>
+                <j:forEach var="install" items="${descriptor.installations}">
+                    <f:option selected="${install.name == instance.ant.name}" value="${install.name}">


BTW we ought to have a standard control in core for selecting a tool installation.

Wadeck changed the title ~~Rewamp~~ Upgrade parent pom Oct 28, 2018

fcojfernandez approved these changes Oct 29, 2018

View reviewed changes

varyvol approved these changes Oct 29, 2018

View reviewed changes

jvz reviewed Oct 29, 2018

View reviewed changes

jvz approved these changes Oct 29, 2018

View reviewed changes

batmat approved these changes Oct 29, 2018

View reviewed changes

jglick previously requested changes Oct 29, 2018

View reviewed changes

Rebase of the PR

a3bb68f

- convert groovy view to jelly - reformat code to follow Java convention - reorder pom file to follow convention https://maven.apache.org/developers/conventions/code.html - rename test variables to follow Jenkins standard - use Jenkins.get()

Wadeck force-pushed the REVAMP_UPGRADE_POM branch from 3d9a0d4 to a3bb68f Compare October 30, 2018 10:02

Wadeck changed the title ~~Upgrade parent pom~~ Rewamp plugin Oct 30, 2018

jglick reviewed Oct 30, 2018

View reviewed changes

Wadeck added 3 commits October 31, 2018 09:46

Remove all the formatting

31f0491

- keep convert groovy view to jelly - keep use Jenkins.get()

Remove all the formatting

59ae71b

Revert the rest of the formatting

8f30525

Wadeck changed the title ~~Rewamp plugin~~ Convert Groovy views to Jelly | use Jenkins.get() Oct 31, 2018

MRamonLeon approved these changes Oct 31, 2018

View reviewed changes

fcojfernandez approved these changes Oct 31, 2018

View reviewed changes

jglick approved these changes Oct 31, 2018

View reviewed changes

jvz approved these changes Oct 31, 2018

View reviewed changes

jglick mentioned this pull request Jul 30, 2019

Adding jglick to ant jenkins-infra/repository-permissions-updater#1240

Merged

5 tasks

oleg-nenashev approved these changes Aug 16, 2019

View reviewed changes

Merge branch 'master' into REVAMP_UPGRADE_POM

182b2b3

jglick added the chore label Aug 27, 2019

jglick merged commit 9b28f6f into jenkinsci:master Aug 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Groovy views to Jelly | use Jenkins.get() #33

Convert Groovy views to Jelly | use Jenkins.get() #33

Wadeck commented Oct 28, 2018 •

edited

Loading

jvz Oct 29, 2018 •

edited

Loading

batmat left a comment

batmat Oct 28, 2018

batmat Oct 29, 2018

batmat Oct 29, 2018

jglick left a comment

jglick Oct 29, 2018

jglick Oct 29, 2018

Wadeck commented Oct 30, 2018

jglick left a comment

jglick Oct 30, 2018

Wadeck Oct 31, 2018

jglick Oct 31, 2018

jglick Oct 30, 2018

jglick Oct 30, 2018

jglick commented Oct 30, 2018

MRamonLeon left a comment

fcojfernandez left a comment

jglick left a comment

jglick Oct 31, 2018

jglick Oct 31, 2018

Wadeck Oct 31, 2018

jglick Oct 31, 2018

	Copyright (c) 2016, CloudBees, Inc.
	Copyright (c) 2018, CloudBees, Inc.

Convert Groovy views to Jelly | use Jenkins.get() #33

Convert Groovy views to Jelly | use Jenkins.get() #33

Conversation

Wadeck commented Oct 28, 2018 • edited Loading

jvz Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

batmat 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

jglick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wadeck commented Oct 30, 2018

jglick 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

jglick commented Oct 30, 2018

MRamonLeon left a comment

Choose a reason for hiding this comment

fcojfernandez left a comment

Choose a reason for hiding this comment

jglick 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

Wadeck commented Oct 28, 2018 •

edited

Loading

jvz Oct 29, 2018 •

edited

Loading