[configuration-as-code] Descriptors configuration

165 views
Skip to first unread message

nicolas de loof

unread,
Nov 9, 2017, 9:45:46 AM11/9/17
to jenkin...@googlegroups.com
This message is a follow up for discussion on https://github.com/jenkinsci/jep/pull/31#discussion_r149598856

configuration-as-code prototype include generic mechanism to configure arbitrary jenkins component relying on @DataBound* ui-binding methods.
but configuration also include Descriptors, and many of them do rely on programmatic data-binding parsing JSONObject. As a sample for this discussion let's consider Mailer plugin build step

This one has attributes, which don't match the UI databound attributes. For sample (for legacy reasons) Java attribute smtpHost is expose on UI as smtpServer (here)

This one uses an optional block in configuration, and as a result it relie son a pseudo boolean attribute useSMTPAuth (== descriptor.smtpAuthUserName!=null) and unset related attributes to null if not selected.

So
  • We can rely on getXXX methods to discover attributes - we can guarantee they're defined 
  • BUT we can't guess psuedo-attributes like useSMTPAuth, neither can we guess the nested JSON document this one involves :
     { "useSMTPAuth" : { "smtpAuthUserName" : ..., "smtpAuthPasswordSecret" : ... } } 

As a resume :
  • We can't invoke @DataBoundSetters for those attributes. Some exist for Mailer bu tin many cases they don't (they're not required by a Descriptor)
  • We can't event directly set attributes using Field.setAccessible hack, as name isn't guaranteed to match (see smtpHost != smtpServer)
  • We can't invoke configure(StaplerRequest, JSONObject) as we can't guess the required nested JSON document structure
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.
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 :'(


Can we just ask them to rely on @DataBound ?
Please remind a Descriptor is a mutable object, i.e if someone un-select some option, the target component won't get attribute reset to null. 

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.

Any thoughts ?




 

Jesse Glick

unread,
Nov 9, 2017, 6:13:25 PM11/9/17
to Jenkins Dev
On Thu, Nov 9, 2017 at 9:45 AM, nicolas de loof
<nicolas...@gmail.com> 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`.

nicolas de loof

unread,
Nov 10, 2017, 3:54:46 AM11/10/17
to jenkin...@googlegroups.com
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 { 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 ?) 

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 only supports immutable components.

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.

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 ?




--
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 jenkinsci-dev+unsubscribe@googlegroups.com.
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.

Jesse Glick

unread,
Nov 13, 2017, 2:26:35 PM11/13/17
to Jenkins Dev
On Fri, Nov 10, 2017 at 3:54 AM, nicolas de loof
<nicolas...@gmail.com> wrote:
> 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 ?)

Maybe. I already did this for `GlobalConfiguration`:

https://github.com/jenkinsci/jenkins/pull/2509

> 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.

Unfortunately your `configure` override must start by setting any
optional struct fields to null:

https://github.com/jenkinsci/ui-samples-plugin/blob/7d0fe2b32204a7fe0ec6adb6815e65d48b0dd54e/src/main/java/jenkins/plugins/ui_samples/HeteroList.java#L81

> 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.

It is not a “possible” race condition; it is definitely a race
condition. Just how Jenkins works. Anyway this is a change you would
apply on Jenkins startup, not while stuff is running, right?

> considering recommended and well adopted approach to
> retrieve a Descriptor is to use
> Jenkins.getInstance.getDescriptor(describable), not referring to some final
> static constant

Some people (following Kohsuke’s lead) `@Inject` descriptors.

> I wonder we could get the Descriptor.configure mechanism to
> rely on @DataBoundConstructors just like Describable do.

I doubt that could be done compatibly. Would break assumptions deep in
`ExtensionList`. Anyway some `Descriptor`s maintain (transient) state,
etc.

Another style is to avoid `f:optionalProperty` and have an explicit
object representing the choice of no authentication, with a no-arg
`@DataBoundConstructor` and a `@Symbol`.

Daniel Beck

unread,
Nov 13, 2017, 3:46:08 PM11/13/17
to jenkin...@googlegroups.com

> On 13. Nov 2017, at 20:26, Jesse Glick <jgl...@cloudbees.com> wrote:
>
>>
>> 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.
>
> Unfortunately your `configure` override must start by setting any
> optional struct fields to null:

Is this just JENKINS-21017 again, or something else?

nicolas de loof

unread,
Nov 13, 2017, 4:09:22 PM11/13/17
to jenkin...@googlegroups.com
This is a comparable issue indeed, even this applies to  updateByXml. not  configure(json)
https://github.com/jenkinsci/jenkins/pull/2736 is a promising approach, but is discovering attributes via reflection, while configuration-as-code is looking at ui-bound parameters. In many case they are the same but not always the case. Anyway this gives me some inspiration to propose a fix.

--
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 jenkinsci-dev+unsubscribe@googlegroups.com.

Kohsuke Kawaguchi

unread,
Nov 14, 2017, 10:19:47 PM11/14/17
to jenkin...@googlegroups.com
On Fri, Nov 10, 2017 at 5:54 PM nicolas de loof <nicolas...@gmail.com> 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 { 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 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.

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

--
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 jenkinsci-de...@googlegroups.com.
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.

For more options, visit https://groups.google.com/d/optout.
--
Kohsuke Kawaguchi

Jesse Glick

unread,
Nov 15, 2017, 9:44:16 AM11/15/17
to Jenkins Dev
On Tue, Nov 14, 2017 at 10:19 PM, Kohsuke Kawaguchi <k...@kohsuke.org> wrote:
> I suppose we could create a variant of req.bindJSON(this,json) that does set
> null on proeprties that are not specified in JSON.

Yes, that would be a helpful convenience API, and it would make sense
to call it in a newly introduced default implementation for
`Descriptor.configure`. For compatibility reasons we could probably
not change the implementation of `GlobalConfiguration.configure`,
unfortunately.

>> 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?

It _would_ be a new problem if you refactored an existing
configuration form to use the recommended idioms and thereby
introduced the race condition.

> When this matters some use of
> 'synchronized' statement is in order anyway.

Would not help. You can synchronize `configure(StaplerRequest,
JSONObject)` so that the nulling out of all properties followed by the
setting of most properties is atomic. But that will not protect
individual `@DataBoundSetter` calls from the proposed C-a-C plugin. We
would need to introduce a scripting-friendly API for configuring an
arbitrary object (create new `Describable` or modify existing
`Descriptor` or `ReconfigurableDescribable`), but without any tie to
Stapler or web forms, along the lines of

http://javadoc.jenkins.io/workflow-step-api/org/jenkinsci/plugins/workflow/steps/StepDescriptor.html#newInstance-java.util.Map-

This could live in `structs` though we would then have a problem with
any affected settings defined in `jenkins-core`.

The more straightforward workaround for such cases is the use of an
explicit object representing a zero choice, as I mentioned previously.
This does have an effect on the UI, at least using the currently
available controls (which would only support a dropdown or a radio
button list, not a checkbox).

nicolas de loof

unread,
Nov 15, 2017, 9:51:55 AM11/15/17
to jenkin...@googlegroups.com
proposed improvement to Descriptor.configure  (WiP) :

tl;dr: 
provide default implementation to Descriptor#configure using databinding. reset descriptor to default values based on properties types default or convention to declare a public {{property}}_default constant. Also updated jelly taglibs to use this same constant in form inputs as default value (until explicitly set)

>>> 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?
>
It _would_ be a new problem if you refactored an existing
configuration form to use the recommended idioms and thereby
introduced the race condition.

No, this race condition already exists while one submit configuration form, whenever we use @databound or not, other threads _already_ can see descriptor in a weird intermediate state. Hopefully one does not update global configuration every millisecond.

--
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 jenkinsci-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr0fLxzZ_4JtTsedFY_-22wdLCJGBuR1e%3DOcKCdD9Zrz%3DA%40mail.gmail.com.

Jesse Glick

unread,
Nov 15, 2017, 10:24:55 AM11/15/17
to Jenkins Dev
On Wed, Nov 15, 2017 at 9:51 AM, nicolas de loof
<nicolas...@gmail.com> wrote:
> proposed improvement to Descriptor.configure (WiP) :
> https://github.com/ndeloof/jenkins/tree/JENKINS-48018

If you are soliciting feedback, just file as a PR and mark as
`work-in-progress`. Easier to gather comments that way.

nicolas de loof

unread,
Nov 15, 2017, 10:27:43 AM11/15/17
to jenkin...@googlegroups.com
this is really work in progress, i.e not even tested on my own before I propose this in a PR :P
was just for information that I was looking into implementing a fix for this idea

--
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 jenkinsci-dev+unsubscribe@googlegroups.com.

nicolas de loof

unread,
Nov 23, 2017, 9:27:19 AM11/23/17
to jenkin...@googlegroups.com
I wrote this documentation for plugin developers to understand the required changes so we can support Descriptor as configuration-as-code targets.
I'll work on writing some pull-requests applying this approach to some major plugins. 

Ewelina Wilkosz

unread,
Nov 24, 2017, 3:51:54 AM11/24/17
to Jenkins Developers
Hi Nicolas, I have a problem with configuring mailer
- if I use "mailer:" as root element Jenkins won't start - exact copy from demo folder
- if I put mailer under "jenkins:" root element there is no error on startup but no configuration is applied

Is there anything special about that one, that should maybe be mentioned in the documentation?



On Thursday, November 23, 2017 at 3:27:19 PM UTC+1, nicolas de loof wrote:
I wrote this documentation for plugin developers to understand the required changes so we can support Descriptor as configuration-as-code targets.
I'll work on writing some pull-requests applying this approach to some major plugins. 
2017-11-15 16:27 GMT+01:00 nicolas de loof <nicolas...@gmail.com>:
this is really work in progress, i.e not even tested on my own before I propose this in a PR :P
was just for information that I was looking into implementing a fix for this idea
2017-11-15 16:24 GMT+01:00 Jesse Glick <jgl...@cloudbees.com>:
On Wed, Nov 15, 2017 at 9:51 AM, nicolas de loof
<nicolas...@gmail.com> wrote:
> proposed improvement to Descriptor.configure  (WiP) :
> https://github.com/ndeloof/jenkins/tree/JENKINS-48018

If you are soliciting feedback, just file as a PR and mark as
`work-in-progress`. Easier to gather comments that way.

--
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 jenkinsci-de...@googlegroups.com.

nicolas de loof

unread,
Nov 24, 2017, 4:19:19 AM11/24/17
to jenkin...@googlegroups.com
yes indeed, for a reason I don't understand yet, "Jenkins.getInstance().getExtensionList(Descriptor.class)" returns an empty list :-\
investigating ...

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/99138212-72cf-47e8-b002-8e00974e1c44%40googlegroups.com.

Ewelina Wilkosz

unread,
Nov 24, 2017, 7:34:31 AM11/24/17
to Jenkins Developers
It looks the list is not empty, it's URL global = ConfigurationAsCode.class.getClassLoader().getResource(cl+"/global.jelly"); that returns null, is it the same issue you're talking about?

nicolas de loof

unread,
Nov 24, 2017, 7:54:10 AM11/24/17
to jenkin...@googlegroups.com
You're right, that was a distinct (non blocking) issue.

this global.jelly lookup was working for me as I had mailer plugin declared as a dependency and ran mvn hpi:run for testing, so this lib is in core classpath
will need to investigate a better way to check a Descriptor has a global.jelly view (I'm so pleased to dig into Stapler internals :P)



To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/eb597cea-7a8e-43ec-a311-42bc44a80c7b%40googlegroups.com.

Ewelina Wilkosz

unread,
Nov 24, 2017, 8:07:56 AM11/24/17
to Jenkins Developers
when I changed

URL global = ConfigurationAsCode.class.getClassLoader().getResource(cl+"/global.jelly");

to

URL global = descriptor.getKlass().toJavaClass().getClassLoader().getResource(cl+"/global.jelly");

it started working for me...

nicolas de loof

unread,
Nov 24, 2017, 8:17:49 AM11/24/17
to jenkin...@googlegroups.com
Yes this is a good short terms workaround.  I'm looking if stapler can easily answer "does this descriptor have a global view" without making assumption on the view template language, but I think your fix is ok for now, please push it to main repository

To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/8f73dced-21c7-4d43-8f55-f41da66c8ea0%40googlegroups.com.

Jesse Glick

unread,
Nov 27, 2017, 5:53:41 PM11/27/17
to Jenkins Dev
On Fri, Nov 24, 2017 at 8:17 AM, nicolas de loof
<nicolas...@gmail.com> wrote:
> I'm looking if stapler can
> easily answer "does this descriptor have a global view" without making
> assumption on the view template language

Try `Descriptor.getGlobalConfigPage`.

nicolas de loof

unread,
Nov 28, 2017, 1:14:21 AM11/28/17
to jenkin...@googlegroups.com
Thanks !

--
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 jenkinsci-dev+unsubscribe@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages