-
Notifications
You must be signed in to change notification settings - Fork 319
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
Fixing groovy configuration script for latest DockerTemplateBase #799
Conversation
This script contains tech-debt - it's calling a deprecated DockerTemplateBase constructor. I would prefer the script changed so that it doesn't use that deprecated constructor at all; that'll then ensure that it doesn't have to be kept in step every time someone introduces anything new. The "official" constructor for DockerTemplateBase takes just the String image; everything else is set by setter methods (which you can call implicitly from groovy). TL;DR: If you're updating this script, make it use the non-deprecated constructor instead of perpetuating the tech-dept so that the issue is fixed once and for all. |
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.
If we're going to fix this file, we should fix it by fixing the tech-debt that caused this problem, rather than just dealing with the symptoms of the problem.
@@ -56,11 +58,13 @@ DockerTemplateBase dockerTemplateBase = new DockerTemplateBase( | |||
dockerTemplateBaseParameters.volumesFromString, | |||
dockerTemplateBaseParameters.environmentsString, | |||
dockerTemplateBaseParameters.hostname, | |||
dockerTemplateBaseParameters.user, |
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.
The constructor used here was deprecated in 2017; we need to stop using it.
Call new DockerTemplateBase(dockerTemplateBaseParameters.image)
and then set all the remaining parameters.
...or try something like dockerTemplateBaseParameters as DockerTemplateBase
or similar groovy "magic" to set everything.
...and either remove or update the // https://github.com/jenkinsci/docker-plugin/blob/docker-plugin-1.1.2/src/main/java/com/nirima/jenkins/plugins/docker/DockerTemplateBase.java
comment that's pointing to an ancient version of the plugin.
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 agree if the DockerTemplateBase constructor is deprecated it makes sense to update the script properly. My groovy skills do not go far enough for that.
I hit upon the script from the official docs but it just errored out.
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.
hey @adamjwsuch, I have created a PR in your fork which addresses deprecation stuff.
Would you be able to review and merge it, please?
…cript update groovy config script to use non-deprecated constructors
Changes to DockerTemplateBase prevent the example groovy script working.
For reference/those searching for the error, like me, it gives this error:
Could not find matching constructor for: com.nirima.jenkins.plugins.docker.DockerTemplateBase
I have made the updates to the groovy script to match the latest DockerTemplateBase