-
-
Notifications
You must be signed in to change notification settings - Fork 300
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
#171 - Package Jenkinsfile Runner as a single JAR file with bundled dependencies + Include SSHD Module to the bundle to fix warning by the Git Server Plugin (Pipeline: Global CPS Library Dependency) #184
Conversation
/** | ||
* This system property is set by the bootstrap script created by appassembler Maven plugin | ||
* to point to a local Maven repository. | ||
*/ | ||
public final File appRepo = new File(System.getProperty("app.repo")); | ||
|
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.
I believe in case of fat jar, the app.repo property will not be provided since repo is part of the jar itself. We've made it on-demand initialization with the help of a function below which will be executed only when collecting jars from repo is required.
if (hasClass(appClassName)) { | ||
Class<?> c = Class.forName(appClassName); | ||
return ((IApp) c.newInstance()).run(this); | ||
} |
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.
We first check if a particular class is present in class path(in case of fat jar), otherwise the old flow will be initiated. i.e. collecting jars and then load the class
Thanks! It looks right. I need to test it locally before merging, I hope to do so by the end of the week |
.make(); | ||
|
||
Class<?> c = cl.loadClass("io.jenkins.jenkinsfile.runner.Runner"); | ||
Class<?> c = bootstrap.hasClass(RUNNER_CLASS_NAME)? Class.forName(RUNNER_CLASS_NAME) : getRunnerClassFromJar(); |
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.
Class<?> c = bootstrap.hasClass(RUNNER_CLASS_NAME)? Class.forName(RUNNER_CLASS_NAME) : getRunnerClassFromJar(); | |
Class<?> c = bootstrap.hasClass(RUNNER_CLASS_NAME) ? Class.forName(RUNNER_CLASS_NAME) : getRunnerClassFromJar(); |
You fetch your class twice here. IMHO it is fine for now, because I do not see a practical future for the old mode, except performance impact which needs to be measured. But this is a case where catching an exception would be better
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.
Hi @oleg-nenashev , do you want me to change it?
@jainsahab So I have merged it with
The Pipeline itself passes successfully. It is already great, thanks a lot! Some discoveries...
|
For 1st point, Let me know what to do, if we want to disable it or not. For 2nd Point, Could you please tell me where exactly we are specifying that dependency so that i can debug why it's not being packaged as part of Jar itself |
Modules are part of the Jenkins Core, and some of them are included in appassembler under |
👍 I will create a follow-up task to get the Git Server disabled and the module removed again |
@oleg-nenashev let me know if there is anything else remaining to be done from my side. |
Retriggering CI |
@oleg-nenashev Could you please help me understand if the build is failing because of the valid reasons? |
Does not look so. CI was pretty unstable over past months. Since we are in Beta, I tend to merge it since local testing works. But I will make another attempt |
Ahh ok, Thanks for the confirmation. :) |
#194 for the CI fix |
@jainsahab could you please merge it with master? |
You mean, merge it with fat_jar branch? |
Thanks @oleg-nenashev for the quick CI fix. All checks have passed and it's green now. |
Thanks a lot for the patch! Will try to release the new Beta version this weekend |
Fix for #171