[JENKINS-63149] - Adding JCasC support #25

Robert-Ilg · 2021-05-11T08:53:36Z

Solves JENKINS-63149: Adding support for JCasC

Details:

Using Descriptor interface instead of Plugin extension
Moving validation of parameters to FormValidation
Adding JCasCCompatibilityTest
Resolving minor compiler warnings and spaces at end of line
Updating pom dependencies
Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

oleg-nenashev · 2021-05-11T09:22:23Z

Thank you! I added it to my review queue.
As a quick feedback, maybe it makes sense to add a short documentation and example to README. The plugin still references Jenkins Wiki in the documentation metadata, but IMHO we will be able to move it to README on GitHub during the next release

damianszczepanik

Comments are left, please review

damianszczepanik · 2021-05-15T13:04:21Z

pom.xml

    </parent>

    <artifactId>sidebar-link</artifactId>
    <packaging>hpi</packaging>
    <name>Sidebar Link</name>
-    <version>1.12.1-SNAPSHOT</version>
-    <url>http://wiki.jenkins-ci.org/display/JENKINS/Sidebar-Link+Plugin</url>
+    <version>2.00.1</version>


is it compatible with v1.12 ?

I'm afraid not.
I don't believe that I managed it to be compatible.

damianszczepanik · 2021-05-15T13:05:51Z

pom.xml

@@ -50,6 +52,31 @@
            <version>1.3</version>
            <scope>test</scope>
        </dependency>
+        <dependency>


do we need joda for this plugin?

I don't know how to solve the plugin conflict in another way:

Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for joda-time:joda-time:2.1 paths to dependency are:
+-org.jenkins-ci.plugins:sidebar-link:2.00.1
+-org.jenkinsci.plugins:pipeline-model-definition:1.8.4
+-org.jenkinsci.plugins:pipeline-model-api:1.8.4
+-com.github.fge:json-schema-validator:2.0.4
+-joda-time:joda-time:2.1
and
+-org.jenkins-ci.plugins:sidebar-link:2.00.1
+-io.jenkins.configuration-as-code:test-harness:1.47
+-com.github.erosb:everit-json-schema:1.12.1
+-com.damnhandy:handy-uri-templates:2.1.8
+-joda-time:joda-time:2.10.2

damianszczepanik · 2021-05-15T13:06:44Z

src/main/java/hudson/plugins/sidebar_link/LinkAction.java

@@ -59,9 +62,14 @@ public LinkAction(String urlName, String displayName, String iconFileName) throw
            throw new IllegalArgumentException(validationResult);
        }

+        if((iconFileName == null) || (iconFileName.equals(""))) {


StringUtils.isBlank(iconFileName) like below

Done: please review "minor changes due to review"

jonesbusy · 2022-01-27T10:54:07Z

Hi! Any chance to have this PR reviewed and merged ? We are interested to use this plugin but are missing jcasc support.

Thanks!

damianszczepanik · 2022-02-05T15:32:18Z

JCasC is merged. I will be happy to release it but before that I need to test it. Who can help me about it? I believe the readme file must be updated as well so it has some simple documentation how to use it

jonesbusy · 2022-02-06T17:59:03Z

@damianszczepanik Amazing! I will test tomorrow the master branch. I will only test that the JcasC configuration is working for the global links (I don't use other feature). Thanks

jonesbusy · 2022-02-07T06:32:05Z

Hi,

I've tested the creation of global links via JcasC. All working as expected

unclassified:
  sidebarGlobalLink:
    links:
    - displayName: "Link1"
      iconFileName: "static/efbf17e4/images/16x16/help.png"
      urlName: "https://www.google.com"
    - displayName: "Link2"
      iconFileName: "static/efbf17e4/images/16x16/help.png"

damianszczepanik · 2022-02-08T21:47:00Z

Published

KalleOlaviNiemitalo · 2022-02-09T14:52:52Z

src/main/java/hudson/plugins/sidebar_link/LinkAction.java

@@ -59,9 +64,14 @@ public LinkAction(String urlName, String displayName, String iconFileName) throw
            throw new IllegalArgumentException(validationResult);
        }

+        if(StringUtils.isBlank(iconFileName)) {
+            iconFileName = "static/efbf17e4/images/16x16/help.png";


This efbf17e4 seems to be SESSION_HASH from https://github.com/jenkinsci/jenkins/blob/5661c1055d026424118e2488b84ea22a19bf423d/core/src/main/java/jenkins/model/Jenkins.java#L5330-L5336. Perhaps the default iconFileName should instead be just "images/16x16/help.png". Reading the current hash from Jenkins.RESOURCE_PATH seems a worse alternative because the resulting iconFileName will be persisted and used after subsequent Jenkins upgrades.

Also, war/src/main/webapp/images/16x16/help.png was deleted from the Jenkins core in jenkinsci/jenkins#5778.

Filed #40 about the default icon.

KalleOlaviNiemitalo · 2022-02-09T14:58:28Z

src/main/java/hudson/plugins/sidebar_link/SidebarLinkPlugin.java

+    @Restricted(NoExternalUse.class)
+    public FormValidation doCheckLinkIcon(@QueryParameter String value) {
+        if (StringUtils.isBlank(value)) {
+            return FormValidation.warning("The provided icon is blank or empty. Defautl will used.");


"Default" not "Defautl"

Was fixed in dd8e66e.

Adding JCasC support

3ef9756

oleg-nenashev self-requested a review May 11, 2021 09:19

oleg-nenashev changed the title ~~Adding JCasC support~~ [JENKINS-63149] - Adding JCasC support May 11, 2021

oleg-nenashev requested a review from damianszczepanik May 11, 2021 09:20

damianszczepanik requested changes May 15, 2021

View reviewed changes

minor changes due to review

1cfee1f

Merge branch 'master' into JCasC

11ec7e9

damianszczepanik merged commit 72f20c6 into jenkinsci:master Feb 5, 2022

KalleOlaviNiemitalo reviewed Feb 9, 2022

View reviewed changes

This was referenced Feb 12, 2022

JENKINS-55710, #28 - Fix icon upload iframe in global config #36

Merged

Migrate the plugin documentation from Wiki to GitHub #21

Open

[JENKINS-68250] Icon defaults to help.png, which was deleted in Jenkins 2.333 #40

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-63149] - Adding JCasC support #25

[JENKINS-63149] - Adding JCasC support #25

Robert-Ilg commented May 11, 2021

oleg-nenashev commented May 11, 2021

damianszczepanik left a comment

damianszczepanik May 15, 2021

Robert-Ilg May 17, 2021

damianszczepanik May 15, 2021

Robert-Ilg May 17, 2021

damianszczepanik May 15, 2021

Robert-Ilg May 17, 2021

jonesbusy commented Jan 27, 2022

damianszczepanik commented Feb 5, 2022

jonesbusy commented Feb 6, 2022

jonesbusy commented Feb 7, 2022

damianszczepanik commented Feb 8, 2022

KalleOlaviNiemitalo Feb 9, 2022

KalleOlaviNiemitalo Feb 9, 2022

KalleOlaviNiemitalo Feb 18, 2022

KalleOlaviNiemitalo Feb 9, 2022

KalleOlaviNiemitalo Feb 13, 2022

[JENKINS-63149] - Adding JCasC support #25

[JENKINS-63149] - Adding JCasC support #25

Conversation

Robert-Ilg commented May 11, 2021

oleg-nenashev commented May 11, 2021

damianszczepanik 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

jonesbusy commented Jan 27, 2022

damianszczepanik commented Feb 5, 2022

jonesbusy commented Feb 6, 2022

jonesbusy commented Feb 7, 2022

damianszczepanik commented Feb 8, 2022

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