On Fri, Nov 10, 2017 at 5:54 PM nicolas de loof <[email protected]> wrote:
> Right, so hopefully we already have docs on best practices for plugin > developers ;) > > An issue remain. Let's consider I refactor Mailer plugin to adopt this > approach (see > https://github.com/ndeloof/mailer-plugin/commit/ffc57e0ff1cb1b74dc1d6fdcb2329a5b9141daaa), > with a new nested optionalProperty describable SMTPAuthentication > <https://github.com/ndeloof/mailer-plugin/blob/master/src/main/java/hudson/tasks/SMTPAuthentication.java> > { username, password } to replace nullable attributes in Mailer$Descriptor > > I'll rewrite configure method as : > > public boolean configure(StaplerRequest req, JSONObject json) throws > FormException { > > req.bindJSON(this, json); > save(); > return true; > } > > > (by the way, shouldn't this be the default implementation ?) > Right. But with this, if I UNCHECK the optional property, no > "authentication" value is posted to JSON form, and I don't get > SMTPAuthentication reset to null in my descriptor. > AFAICT Describable mechanism documented here > <https://jenkins.io/doc/developer/plugin-development/pipeline-integration> > only > supports immutable components. > Aha! That is indeed a problem. I suppose we could create a variant of req.bindJSON(this,json) that does set null on proeprties that are not specified in JSON. If we are updating an existing instance I think it's a reasonable behaviour to expect. I think this is easier local change than creating a new Descriptor instance upon config resubmission. Does this mean Descriptors would need to include an extra "reset" method to > set all attributes to null / default value ? > This also make me think this configuration mechanism isn't atomic, and > there's some possible race condition for related attributes to have > inconsistent values seen from another thread while the configuration is > being changed. > This isn't a new problem, right? When this matters some use of 'synchronized' statement is in order anyway. As a resume, and considering recommended and well adopted approach to > retrieve a Descriptor is to use > Jenkins.getInstance.getDescriptor(describable), not referring to some final > static constant, I wonder we could get the Descriptor.configure mechanism > to rely on @DataBoundConstructors just like Describable do. > This means we will need to (atomically) swap Descriptor instance in > ExtensionList. Maybe this has too much impact (extensionLists Memoizer > would need to be reset) ? Or maybe there's a better way to support > re-configuration with @DataBound ? > > > > 2017-11-10 0:13 GMT+01:00 Jesse Glick <[email protected]>: > >> On Thu, Nov 9, 2017 at 9:45 AM, nicolas de loof >> <[email protected]> wrote: >> > As a sample for this >> > discussion let's consider Mailer plugin build step >> >> And the comment >> >> > this code is brain dead >> >> :-) >> >> > Until you have some clever hack to suggest, it looks to me we will need >> to >> > provide guidance for plugin developers for "best practice to implement >> > Descriptor" so it can be used by configuration-as-code. >> >> A lot of this is actually identical to the constraints needed to make >> code compatible with *Pipeline Syntax*, and more broadly with the >> `DescribableModel` API. See: >> >> >> https://jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters >> >> > I sounds to me we don't have any way to provide a generic mechanism >> without >> > changes to Descriptors in core/plugins - yes I known, this means >> hundreds >> > pull-requests >> >> Start filing them… >> >> The main difficulty is not in writing correct code, which is in fact >> usually easier than the old way, it is in maintaining deserialization >> compatibility. So you need to use `readResolve` and occasionally even >> XStream tricks to load old, awkward property layouts into a logical >> revised structure. >> >> > To reuse Mailer$Descriptor sample, this would require useSMTPAuth to be >> an >> > actual nested data structure, not just a boolean flag to configure some >> > optional attributes. This means that it would have to define a nested >> java >> > class to own smtpAuthUserName + smtpAuthPasswordSecret attributes. >> >> Yes. `f:optionalProperty` and a nested `Describable`. See for example >> `HeteroList` in `ui-samples`. >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Jenkins Developers" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr3Vq3AVbvMFQDiB2xApKiqT%3Dswcsr%3D606V5OuKVrkKZmA%40mail.gmail.com >> . >> For more options, visit https://groups.google.com/d/optout. >> > > -- > You received this message because you are subscribed to the Google Groups > "Jenkins Developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJz%3DDrRF%3DXM8rp0A-zyt4D6wTXTBU2eQ_YFZ%3DHjQyHxzGpw%40mail.gmail.com > <https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJz%3DDrRF%3DXM8rp0A-zyt4D6wTXTBU2eQ_YFZ%3DHjQyHxzGpw%40mail.gmail.com?utm_medium=email&utm_source=footer> > . > For more options, visit https://groups.google.com/d/optout. > -- Kohsuke Kawaguchi -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CAN4CQ4z2T98OBJ5u_bDPD8%3Db0_3JrK3T4wfND4J2diMvwUS-bQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
