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.

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

Support icons in folders #1258

Merged
merged 14 commits into from
Mar 22, 2023
Merged

Support icons in folders #1258

merged 14 commits into from
Mar 22, 2023

Conversation

strangelookingnerd
Copy link
Contributor

This PR enables the icon configuration on folders, multi-branch projects and organization folders to be accessible. This way one can customize the folder icon via jenkinsci/custom-folder-icon-plugin.
See jenkinsci/custom-folder-icon-plugin#92 as well for details and use case.

image

Here the jenkinsci/custom-folder-icon-plugin is installed, by default there is only stockFolderIcon and metaDataActionFolderIcon available.

Example configuration:

folder('stock')

userContent('customFolderIcons/custom.png', streamFileFromWorkspace('custom.png'))
folder('custom') {
  icon {
    customFolderIcon {
      foldericon('custom.png')
    } 
  }
}
folder('ionicon') {
  icon {
    ioniconFolderIcon {
      ionicon('jenkins')
    } 
  }
}
folder('build-status') {
  icon {
      buildStatusFolderIcon()
  }
}

@strangelookingnerd
Copy link
Contributor Author

As for the failing test, I have no idea how to fix it if I'm honest other than removing the example. Any hints are highly appreciated.

@jonesbusy
Copy link
Contributor

@jamietanna Can you take a look please ?

@jonesbusy
Copy link
Contributor

Ping @jenkinsci/job-dsl-plugin-developers

@jamietanna jamietanna self-requested a review October 29, 2022 10:22
@strangelookingnerd
Copy link
Contributor Author

@jamietanna Any update on this? Just wondering if there is anything I could do to get this merged.

@quilicicf
Copy link

Stack trace from failing test is:

Expected no exception to be thrown, but got 'javaposse.jobdsl.dsl.DslScriptException'
	at spock.lang.Specification.noExceptionThrown(Specification.java:118)
	at javaposse.jobdsl.dsl.doc.ExampleValidationSpec.test examples in #file(ExampleValidationSpec.groovy:32)
Caused by: javaposse.jobdsl.dsl.DslScriptException: (script, line 8) No signature of method: javaposse.jobdsl.dsl.helpers.icon.FolderIconContext.customFolderIcon() is applicable for argument types: (script$_run_closure1$_closure4$_closure5) values: [script$_run_closure1$_closure4$_closure5@78eced16]
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at javaposse.jobdsl.dsl.AbstractDslScriptLoader.runScriptEngine(AbstractDslScriptLoader.groovy:114)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at javaposse.jobdsl.dsl.AbstractDslScriptLoader.runScripts_closure1(AbstractDslScriptLoader.groovy:61)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at groovy.lang.Closure.call(Closure.java:414)
	at groovy.lang.Closure.call(Closure.java:430)
	at javaposse.jobdsl.dsl.AbstractDslScriptLoader.runScripts(AbstractDslScriptLoader.groovy:46)
	at javaposse.jobdsl.dsl.doc.ExampleValidationSpec.test examples in #file(ExampleValidationSpec.groovy:29)
Caused by: groovy.lang.MissingMethodException: No signature of method: javaposse.jobdsl.dsl.helpers.icon.FolderIconContext.customFolderIcon() is applicable for argument types: (script$_run_closure1$_closure4$_closure5) values: [script$_run_closure1$_closure4$_closure5@78eced16]
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at javaposse.jobdsl.dsl.AbstractExtensibleContext.methodMissing(AbstractExtensibleContext.groovy:19)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at script.run_closure1$_closure4(script:8)
	at script.run_closure1$_closure4(script)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at javaposse.jobdsl.dsl.ContextHelper.executeInContext(ContextHelper.groovy:16)
	at javaposse.jobdsl.dsl.AbstractFolder.icon(AbstractFolder.groovy:66)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at script.run_closure1(script:7)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at groovy.lang.Closure.call(Closure.java:414)
	at groovy.lang.Closure.call(Closure.java:430)
	at javaposse.jobdsl.dsl.JobParent.processItem(JobParent.groovy:238)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at groovy.lang.GroovyObjectSupport.invokeMethod(GroovyObjectSupport.java:46)
	at groovy.lang.Script.invokeMethod(Script.java:80)
	at javaposse.jobdsl.dsl.JobParent.folder(JobParent.groovy:174)
	at script.run(script:6)
	at javaposse.jobdsl.dsl.AbstractDslScriptLoader.runScript(AbstractDslScriptLoader.groovy:138)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at javaposse.jobdsl.dsl.AbstractDslScriptLoader.runScriptEngine(AbstractDslScriptLoader.groovy:108)
	... 7 more

I'd wager that the custom icon plugin is not installed in the test instance and therefore the test script cannot be applied.

The problem is that I don't know how to fix this, a couple ideas:

  • make the plugin available on the test instance, but no other non-core plugins are installed, which is likely on purpose. This points to either:
    • having this plugin added in core (not likely and probably not desirable)
    • installing it in the build.gradle file (but it's likely not what the Job DSL plugin maintainers have in mind, they probably want to avoid pulling complexity from non-core plugins in their own)
  • update the custom folder icon plugin so that its configuration does not clash with that of the Folder plugin which is from the core and installed on all instances. I'm not sure this is the right option either, can the custom folder icon plugin overwrite settings from the Folder plugin? Was the Folder plugin planning for its configuration to be over-written? 🤔

Unfortunately, I'm far from a Gradle pro and I didn't manage to tweak this project's build.gradle file to test my hypothesis 😞

@strangelookingnerd
Copy link
Contributor Author

strangelookingnerd commented Jan 29, 2023

@quilicicf sums it up perfectly.

I'd wager that the custom icon plugin is not installed in the test instance and therefore the test script cannot be applied.

I do second this, however I yet have to find a way to change that.

make the plugin available on the test instance, but no other non-core plugins are installed, which is likely on purpose. This points to either:

  • having this plugin added in core (not likely and probably not desirable)
  • installing it in the build.gradle file (but it's likely not what the Job DSL plugin maintainers have in mind, they probably want to avoid pulling complexity from non-core plugins in their own)

I would prefer the 2nd option where my plugin is only pulled as a "test dependency" but should by no means be a required dependency of the job-dsl-plugin otherwise.

update the custom folder icon plugin so that its configuration does not clash with that of the Folder plugin which is from the core and installed on all instances. I'm not sure this is the right option either, can the custom folder icon plugin overwrite settings from the Folder plugin? Was the Folder plugin planning for its configuration to be over-written? 🤔

My plugin uses the intended way of extending the Folders plugin and provides additional functionality alongside its own configuration. There is no clash in any way. To elaborate further, the "icon" property of a folder is intended to be overwritten but this was not foreseen in the job-dsl-plugin implementation - which is exactly what this PR is trying to change.

Unfortunately, I'm far from a Gradle pro and I didn't manage to tweak this project's build.gradle file to test my hypothesis 😞

I'm in the same boat as you and since there is basically little to no documentation for the gradle build process of Jenkins plugins or other plugin projects using gradle to look at I desperately hope for @jamietanna to help me out here and get this PR merged.

@quilicicf
Copy link

It would be super nice if the Job DSL plugin allowed extensions like this indeed.
Let's wait for maintainers to answer then.

@jonesbusy
Copy link
Contributor

Apparently the plugin will switch to maven build (#1265)

@strangelookingnerd
Copy link
Contributor Author

@jonesbusy I will look into this PR again once #1265 is merged ✌🏼

@jamietanna
Copy link
Contributor

Thanks folks, sorry for the delay, but with the Maven changes in it may resolve the build issues 🤞🏽

@strangelookingnerd strangelookingnerd requested a review from a team as a code owner March 18, 2023 10:31
}

// use https://github.com/jenkinsci/custom-folder-icon-plugin for ionicons
folder('ionicon') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@strangelookingnerd Should we also have a test for new emoji type icons ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

job-dsl-plugin requires Jenkins 2.361.4 whereas custom-folder-icon-plugin 2.5 (which includes the emoji icons) requires 2.387.1 - so I guess this won't be possible right now.

Copy link
Member

Choose a reason for hiding this comment

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

I can probably get a release out next week. Once that's done I would have no problem with revving the minimum Jenkins core to 2.387.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@basil That would be great! On a side-note I found that the tests in javaposse.jobdsl.dsl.doc.ExampleValidationSpec do not seem to be run during the build anymore or are at least not reporting any failing tests. Running them manually however shows that the test for my icon.groovy example still fails. Before the switch to Maven the behavior was different. Any idea why that is?

Copy link
Member

@basil basil Mar 19, 2023

Choose a reason for hiding this comment

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

I think it has something to do with the use of @Unroll with file << findAllExamples(), where the right-hand side is a function call rather than a static set of values. Seems that JUnit isn't able to invoke the function call correctly. Are you interested in opening a PR that removes the use of @Unroll in favor of a simple for loop inside of the test?

Copy link
Member

Choose a reason for hiding this comment

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

Remove icon examples in doc as it does not work by any means
We keep a link to custom-folder-icon
@strangelookingnerd
Copy link
Contributor Author

@jamietanna I cannot get the ExampleValidationSpec test for the icon.groovy to work the way I want it to. I have no idea why that is and I'm giving up on it for now. Unless you can give me any pointer on what I'm missing, I simply can not put more time into it. I removed the example that uses the custom-folder-icon-plugin and only left the default example.
However I verified manually that everything is working just fine: Once the custom-folder-icon-plugin is installed I have the full selection of implementations available and all of them can be configured and do work just as described in my initial comment.

I would still love to see this merged eventually.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I have not reviewed this change thoroughly. At minimum it is missing @RequiresPlugin/@RequiresPlugins and the icon examples need to be restored.

@strangelookingnerd
Copy link
Contributor Author

I have not reviewed this change thoroughly. At minimum it is missing @RequiresPlugin/@RequiresPlugins and the icon examples need to be restored.

@basil The icon property is provided by the Folders plugin and is not something introduced by the custom-folder-icon. So @RequiresPlugin needs to only be set to the Folders plugin, which is already the case.
For the example I tried everything that came to my mind over the course of now almost 6 month. I was unable to come up with any solution that does work other than disabling the test that fails because of it. As I already said, it would be great to have the examples included but unless someone comes up with an idea why it is not working and / or how to fix it there is nothing I can do.
The test setup with groovy, mocking via Spock and little to no docs / examples even drove me towards looking for a solution via ChatGPT, but it failed me as well. 🤷🏼

@basil
Copy link
Member

basil commented Mar 21, 2023

I took a closer look. It looks like FolderPropertiesContext and FolderIconContext are defined in the old-style DSL but extended by the Dynamic DSL. This is inconsistent, but since it is a preëxisting issue I will not insist that you fix it in this PR.

Since the Dynamic DSL is not available in job-dsl-core, the only way for you to test this is in job-dsl-plugin. Please add custom-folder-icon in test scope to job-dsl-plugin/pom.xml and then write tests for this in javaposse.jobdsl.plugin.ExecuteDslScriptsSpec. In this environment, the custom-folder-icon plugin will be loaded by JenkinsRule and will be available for the Dynamic DSL.

(No LLMs were consulted during the writing of this answer.)

@strangelookingnerd
Copy link
Contributor Author

@basil Thanks again for you help. As requested I added tests for every icon type we currently have, excluding the EmojiFolderIcon which I will add once the baseline is bumped to 2.387.1.

@strangelookingnerd strangelookingnerd requested review from basil and removed request for jamietanna March 21, 2023 21:23
@basil basil mentioned this pull request Mar 21, 2023
@basil
Copy link
Member

basil commented Mar 21, 2023

which I will add once the baseline is bumped to 2.387.1.

#1288

@strangelookingnerd
Copy link
Contributor Author

Added a test for the EmojiFolderIcon as well 👍🏼

@basil basil changed the title Enable configuration for icons in folders Support icons in folders Mar 22, 2023
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thanks!

@basil basil merged commit 2b2e3f7 into jenkinsci:master Mar 22, 2023
@basil
Copy link
Member

basil commented Mar 22, 2023

Released in 1.83.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants