Switch job configuration section to use a checkbox #88
Conversation
|
Diabol » delivery-pipeline-plugin #398 SUCCESS |
|
Thanks for the contribution! Is there a JIRA ticket for this issue? Otherwise please file a JIRA at https://issues.jenkins-ci.org/ and mark it as component "delivery-pipeline" so we can keep track of all the changes going in to the plugin. I would also suggest squashing the commits and amend the commit message with the associated ticket id. |
|
Thanks for the comments Tommy! |
|
|
||
| @Exported | ||
| public boolean getConfigEnabled() { | ||
| return configEnabled; |
There was a problem hiding this comment.
Thanks for the PR!
I think it is possible to remove the need for adding a new member for this class than also needs to be serialized to the configuration file of the project. It could break the Jenkins Job builder and Job DSL plugin project generation.
Is it possible to remove the configEnabled member instead have this in the method?
return taskName != null && !taskName.equals("") || stageName != null && !stageName.equals("");
There was a problem hiding this comment.
I agree with Patrik in principle, but please don't put four boolean expressions in a row like that ;)
Don't we have any string util class on the classpath?
And don't forget the tests.
Marcus Philip
+46 70 767 46 77
Twitter: @marcus_phi
22 nov 2014 kl. 13:05 skrev Patrik Boström notifications@github.com:
In src/main/java/se/diabol/jenkins/pipeline/PipelineProperty.java:
@@ -52,6 +58,11 @@ public String getTaskName() {
public String getStageName() {
return stageName;
}
- @exported
- public boolean getConfigEnabled() {
return configEnabled;
I think it is possible to remove the need for adding a new member for this class than also needs to be serialized to the configuration file of the project. It could break the Jenkins Job builder and Job DSL plugin project generation.
Is it possible to remove the configEnabled member instead have this in the method?return taskName != null && !taskName.equals("") || stageName != null && !stageName.equals("");—
Reply to this email directly or view it on GitHub.
There was a problem hiding this comment.
I think I could probably get rid of the need for configEnabled altogether.
Which means I should be able to remove the method, the new field and the new constructor argument.
But it's still possible it will break the plugins you mentioned as it would still require configEnabled to be included in the StaplerRequest object being passed into the newInstance method. This is because when the job config is saved, we need to have at least one parameter to know if the checkbox is ticked or not.
I have almost no knowledge of how these plugins work though, so I could be very wrong here.
Regarding the tests, I have updated the existing ones and added a new one to test the new constructor parameter.
|
Diabol » delivery-pipeline-plugin #399 SUCCESS |
Normal convention for Jenkins plugins which contribute a JobProperty to a jobs configuration page is to use an optionalBlock as the top level jelly tag. In a jobs properties section of the configuration page, plugins 'hide' their configuration entries by using the optionalBlock, this is presented to the user as a checkbox. This keeps the jobs configuration page quite clean while allowing the user full control over the properties they use.
|
Diabol » delivery-pipeline-plugin #400 SUCCESS |
Switch job configuration section to use a checkbox
|
Thanks for the PR! |
|
It's a great plugin and I'm only happy to be able to help :) |
This pull request changes the plugins job configuration from using a section to an optionalBlock. In effect, it makes the plugins configuration fall inline with the convention used by most other plugins.