Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-57513

Split the large databound constructors into databound setters

      Inside EC2 plugin there is a lot of constructors with a large set of databound constructors.
      Databound constructor should only be used for mandatory/final parameters

      Where databound setters should be used for optional parameters.

      https://jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters

          [JENKINS-57513] Split the large databound constructors into databound setters

          Matt Sicker added a comment -

          I'd love to get that merged, though my Stapler street cred hasn't been enough yet to gain committership yet. I hope to change that over the next couple months, though.

          Matt Sicker added a comment - I'd love to get that merged, though my Stapler street cred hasn't been enough yet to gain committership yet. I hope to change that over the next couple months, though.

          Joseph Petersen added a comment - EC2 is still going down a dark road as this is still happening. https://github.com/jenkinsci/ec2-plugin/blame/master/src/main/java/hudson/plugins/ec2/SlaveTemplate.java

          raihaan please take a look

          Joseph Petersen added a comment - raihaan  please take a look

          Hey Joseph, I agree we really need to fix this issue.

          The whole class(SlaveTemplate) needs a rework so that not everything is final so setters would work.

           

          jvz I'm not sure how builder patterns will help this since I don't think casc will support a builder

          Raihaan Shouhell added a comment - Hey Joseph, I agree we really need to fix this issue. The whole class(SlaveTemplate) needs a rework so that not everything is final so setters would work.   jvz I'm not sure how builder patterns will help this since I don't think casc will support a builder

          Matt Sicker added a comment -

          A builder pattern is the proper Java way to do it. Since nobody has implemented support for that in the JSON binding in Stapler, it seems like the suggested pattern is to use 90's-style JavaBeans with mutable getters/setters. Essentially, any data binding parameters that are required should be in the constructor, and any that are optional (i.e., every single new option added since the first release technically) should be added as setters so that the class can still be designed to work if that setter isn't invoked (i.e., providing default behavior).

          Matt Sicker added a comment - A builder pattern is the proper Java way to do it. Since nobody has implemented support for that in the JSON binding in Stapler, it seems like the suggested pattern is to use 90's-style JavaBeans with mutable getters/setters. Essentially, any data binding parameters that are required should be in the constructor, and any that are optional (i.e., every single new option added since the first release technically) should be added as setters so that the class can still be designed to work if that setter isn't invoked (i.e., providing default behavior).

          Can you please leave the old constructors in place. I'm already using groovys scripts to configure the plugin.
          Splitting out into setters is a welcome change, but it sucks to have to update your scripts for a new release.

          Jakub Bochenski added a comment - Can you please leave the old constructors in place. I'm already using groovys scripts to configure the plugin. Splitting out into setters is a welcome change, but it sucks to have to update your scripts for a new release.

          Why not offer a builder pattern and proper data bound setters, so that those in groovy can use the builder pattern and JCasC and stapler can use data binding.

          Joseph Petersen added a comment - Why not offer a builder pattern and proper data bound setters, so that those in groovy can use the builder pattern and JCasC and stapler can use data binding.

          Matt Sicker added a comment -

          The point of the builder pattern was to allow for a single (private) mega-constructor that continues to change as the class changes, but centralizing the configuration in its own builder class. Alternatively, if all these parameters are moved into their own anemic domain model class, then having everything be set up via getters and setters wouldn't be as painful. The general idea here is about setting up the state of the object by the time it's constructed so that you can avoid representations of invalid state.

          Matt Sicker added a comment - The point of the builder pattern was to allow for a single (private) mega-constructor that continues to change as the class changes, but centralizing the configuration in its own builder class. Alternatively, if all these parameters are moved into their own anemic domain model class, then having everything be set up via getters and setters wouldn't be as painful. The general idea here is about setting up the state of the object by the time it's constructed so that you can avoid representations of invalid state.

          We might have to make a breaking change here since everything is public to give jcasc its databinding we need to remove the final keyword but since fields are public they now become directly modifiable. We probably should bite the bullet and change them to be private and provide setters / getters? WDYT: jvz thoulen

          Raihaan Shouhell added a comment - We might have to make a breaking change here since everything is public to give jcasc its databinding we need to remove the final keyword but since fields are public they now become directly modifiable. We probably should bite the bullet and change them to be private and provide setters / getters? WDYT: jvz thoulen

          Matt Sicker added a comment -

          Due to how many people already have pipelines referring to those constructors, it might be worth trying to deprecate that class entirely with a compatibility layer to the new class.

          Matt Sicker added a comment - Due to how many people already have pipelines referring to those constructors, it might be worth trying to deprecate that class entirely with a compatibility layer to the new class.

            thoulen FABRIZIO MANFREDI
            casz Joseph Petersen (old)
            Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated: