-
Notifications
You must be signed in to change notification settings - Fork 830
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
Remove <optional> from config-file-provider dependency #1414
Remove <optional> from config-file-provider dependency #1414
Conversation
@jenkinsci/job-dsl-plugin-developers any chance this can be reviewed, merged & released? |
Not a maintainer of What about job-dsl-plugin/job-dsl-plugin/src/main/java/javaposse/jobdsl/plugin/ExecuteDslScripts.java Line 611 in e6d655d
This check if the plugin is installed Also here : job-dsl-plugin/job-dsl-plugin/src/main/java/javaposse/jobdsl/plugin/JenkinsDslScriptLoader.java Line 26 in e6d655d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin seems to comply with https://www.jenkins.io/doc/developer/plugin-development/optional-dependencies/ before this PR.
This is true, however I'd argue that the ExecuteDslScripts and JenkinsDslScriptLoader can be considered core functionallities of this plugin. If these fail to load in case the optional dependency is not present, I don't see the point in them being optional. |
Based on the |
I was under the assumption, yes. However I fail to reproduce the error from jenkinsci/custom-folder-icon-plugin#280 / jenkinsci/maven-hpi-plugin#391 Closing the PR 👍🏼 |
Remove the
optional
flag from theconfig-file-provider
dependency.To my understanding this dependency is directly referenced in
ExecuteDslScripts
as well asJenkinsDslScriptLoader
and for that reason should not be optional.Resolves jenkinsci/custom-folder-icon-plugin#280
Testing done
Ran existing tests without any errors.
Submitter checklist