[JEP-201] [configuration-as-code] Feedback from Meetup

[Jesse Glick]

Some things that might otherwise have gotten lost in IRC.


About the automatic inference of symbols like `ldap` from
`hudson.security.LDAPSecurityRealm`: this seems like a nice trick to
use when prototyping the JEP, so you can see realistic stuff working
before plugins are updated to use `@Symbol`. But I do not think this
should be retained in a 1.0 version—all supported plugins should be
given the annotation. Reasoning:

· Adding an annotation poses no risk to plugin stability, so there is
no reason for a maintainer to object to it.
· Symbols must be unique within their extension point (for example
there may be only one `SecurityRealm` named `ldap`), so they must be
chosen carefully and deliberately.
· People might begin relying on the inferred names and then it would
be a compatibility issue to fix this later.


The credentials domain syntax, IIRC something like

system:
? # global
: -credentials
# …

baffles everyone who sees it. Just because internally `credentials`
uses a `Map` from (I guess) domains to lists of credentials does not
mean you must reflect that in YAML syntax. You should rather model a
list of credentials, each of which may have an optional attribute for
the domain (ID?).

It is no excuse that you are trying to keep things mapped tightly to
the databinding model in the plugin, because Jenkins databinding does
not have any standard support for `Map`s at all—this is all bound to
custom UI screens in `credentials`. Therefore it is reasonable for
JCasC to also have a custom converter. Plugins which use simple
databinding, which would consist of `List`s of `Describable`s only,
would not need any custom code.


Using Job DSL Groovy to create jobs is fine as a short-term workaround
but should not be how `TopLevelItem`s are defined in 1.0 I think.
There should be native YAML syntax for this, based on the same
databinding model as any other settings. As `@Symbol` is adopted,
these should look structurally similar anyway.

[nicolas de loof]

2018-04-17 17:58 GMT+02:00 Jesse Glick <jgl...@cloudbees.com>:

Some things that might otherwise have gotten lost in IRC.


About the automatic inference of symbols like `ldap` from
`hudson.security.LDAPSecurityRealm`: this seems like a nice trick to
use when prototyping the JEP, so you can see realistic stuff working
before plugins are updated to use `@Symbol`. But I do not think this
should be retained in a 1.0 version—all supported plugins should be
given the annotation. Reasoning:

· Adding an annotation poses no risk to plugin stability, so there is
no reason for a maintainer to object to it.
· Symbols must be unique within their extension point (for example
there may be only one `SecurityRealm` named `ldap`), so they must be
chosen carefully and deliberately.
· People might begin relying on the inferred names and then it would
be a compatibility issue to fix this later.

the yaml schema is for a specific version of jenkins-core + plugin. 

Any change to a plugin will change this model, this will happen any time a DataBoundConstructor is modified, this is the price to pay for allignment with UI databinding.

As a result, I don't think we should expect any compatibility on version upgrades. If you want to run with new version of plugins, run a test master or any JCasC validation tool before.

 

The credentials domain syntax, IIRC something like

system:
  ? # global
  : -credentials
    # …

baffles everyone who sees it. Just because internally `credentials`
uses a `Map` from (I guess) domains to lists of credentials does not
mean you must reflect that in YAML syntax. You should rather model a
list of credentials, each of which may have an optional attribute for
the domain (ID?).

The idea here was to estimate the minimal required code base to offer glue code for credentials. This is definitively not for 1.0

I would anyway expect Credentials plugin to own this code, so such decision is made from maintainer - I don't understand why this plugin adopted this odd design - most probably for compatibility reasons - the web UI offering a hierarchical representation of registered credentials.

 

It is no excuse that you are trying to keep things mapped tightly to
the databinding model in the plugin, because Jenkins databinding does
not have any standard support for `Map`s at all—this is all bound to
custom UI screens in `credentials`. Therefore it is reasonable for
JCasC to also have a custom converter. Plugins which use simple
databinding, which would consist of `List`s of `Describable`s only,
would not need any custom code.

As I said, I expect such things to take place within plugins themsleves, either with custom JCasC _or_ by changing the databound model

 

Using Job DSL Groovy to create jobs is fine as a short-term workaround
but should not be how `TopLevelItem`s are defined in 1.0 I think.
There should be native YAML syntax for this, based on the same
databinding model as any other settings. As `@Symbol` is adopted,
these should look structurally similar anyway.

Agree, sounds to me job-dsl could support a yaml syntax (just by switching parser). Need to investigate how to generate the adequate schema and documentation

 

--
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/CANfRfr3e56PQf4vrDLPjPU11MxzKX3L%3DR%2BQVROgyAyuKRYB97A%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Jesse Glick]

On Tue, Apr 17, 2018 at 2:03 PM, nicolas de loof
<nicolas...@gmail.com> wrote:
> the yaml schema is for a specific version of jenkins-core + plugin.
> Any change to a plugin will change this model, this will happen any time a
> DataBoundConstructor is modified

Well, typically we would expect parameters to be compatible after a
plugin update, so that if for example a `@DataBoundSetter` method
needs to be `@Deprecated`, it is removed from databinding but still
accessible as a Java setter. The compatibility policy for such
refactorings does need to be defined. Currently we only expect plugins
to be compatible in their XStream settings form, plus Pipeline `Step`s
and any `Describable`s used by them need to continue to accept
parameters that worked before.

> I would anyway expect Credentials plugin to own this code, so such decision
> is made from maintainer

JEP-201 is introducing a new overall Jenkins feature, and the
Credentials plugin is a longstanding piece of plumbing, so I would
expect JEP-201 developers to address this particular integration.

> I don't understand why this plugin adopted this
> odd design - most probably for compatibility reasons

I do not think compatibility had anything to do with it. The
configuration model of Credentials is that you have various providers,
inside of each of which you can create various domains, each of which
then contains a set of credentials. The Java-level structure is more
or less arbitrary and was not intended to map directly to Jenkins
databinding, because its UI manifestation is not a single
configuration screen.

> sounds to me job-dsl could support a yaml syntax (just by switching
> parser)

Why would you need `job-dsl` at all? You already have a general system
for binding YAML to `Describable`s. You would need a bit of extra code
to bind some syntax to `TopLevelItem` and
`DirectlyModifiableTopLevelItemGroup`, and a call to JENKINS-50173 if
and when written.

[nicolas de loof]

2018-04-17 22:05 GMT+02:00 Jesse Glick <jgl...@cloudbees.com>:

On Tue, Apr 17, 2018 at 2:03 PM, nicolas de loof
<nicolas...@gmail.com> wrote:
> the yaml schema is for a specific version of jenkins-core + plugin.
> Any change to a plugin will change this model, this will happen any time a
> DataBoundConstructor is modified

Well, typically we would expect parameters to be compatible after a
plugin update, so that if for example a `@DataBoundSetter` method
needs to be `@Deprecated`, it is removed from databinding but still
accessible as a Java setter. The compatibility policy for such
refactorings does need to be defined. Currently we only expect plugins
to be compatible in their XStream settings form, plus Pipeline `Step`s
and any `Describable`s used by them need to continue to accept
parameters that worked before.

we exclude Deprecated setters as potential CasC attributes, so this wouldn't help

As you said there's no expectation for compatibility at this level we could rely on.

That being said I don't consider this to be an issue : with CasC we can produce a schema and tooling to discover such compatibility issue and let end-user know before they apply to production.

 

> I would anyway expect Credentials plugin to own this code, so such decision
> is made from maintainer

JEP-201 is introducing a new overall Jenkins feature, and the
Credentials plugin is a longstanding piece of plumbing, so I would
expect JEP-201 developers to address this particular integration.

CasC plugin demonstrate we can support it with current implementation, even if this one relies on some uncommon yaml syntax. 

I consider credentials-plugin maintainer to have a better vision of what should be exposed to end-user than me.

 

> I don't understand why this plugin adopted this
> odd design - most probably for compatibility reasons

I do not think compatibility had anything to do with it. The
configuration model of Credentials is that you have various providers,
inside of each of which you can create various domains, each of which
then contains a set of credentials. The Java-level structure is more
or less arbitrary and was not intended to map directly to Jenkins
databinding, because its UI manifestation is not a single
configuration screen.

> sounds to me job-dsl could support a yaml syntax (just by switching
> parser)

Why would you need `job-dsl` at all? You already have a general system
for binding YAML to `Describable`s. You would need a bit of extra code
to bind some syntax to `TopLevelItem` and
`DirectlyModifiableTopLevelItemGroup`, and a call to JENKINS-50173 if
and when written.

1. Job class hierarchy is full of hand written JSON parsing so we can't use same discovery approach

2. job-dsl is very popular for this purpose, I don't want to spend time re-inventing the wheel for a successful solution.

I'd like job-dsl support to be option in JCasC, so we do support this approach but some alternate way is also possible in a future release.

--
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/CANfRfr00%2BpNy%2BSfbY%2BLeye-GYd0v7Q%2BxttRZCJddyv_Y8fgOyA%40mail.gmail.com.

[Jesse Glick]

On Tue, Apr 17, 2018 at 4:45 PM, nicolas de loof
<nicolas...@gmail.com> wrote:
> Job class hierarchy is full of hand written JSON parsing

I suspect such cases are fixable, which would take some work, but on
the other hand we would get a clearer code base as a result anyway.

[nicolas de loof]

Sure, but this will be for future version then

We want JCasC to support current releases of Jenkins by Praqma customers (and others), not require bleeding edge Jenkins core.

 

--
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/CANfRfr03rVj6r1tWfwYeO1QKeDMtDYjuc8fF0Se%2B4Rh9qbZFvg%40mail.gmail.com.

[Ewelina Wilkosz]

interesting discussion!

I agree we can have a nicer way of configuring job, to keep the consistency, but I also agree with Nicolas about not wanting to "re-invent the wheel" - many users have their job dsls ready, so the transition may be easier if they don't need to learn a new syntax, that was my motivation for keeping job dsl. Alternative solution may exist next to job dsl support I believe

On Wednesday, April 18, 2018 at 6:53:58 AM UTC+2, nicolas de loof wrote:

2018-04-18 0:42 GMT+02:00 Jesse Glick <jgl...@cloudbees.com>:

On Tue, Apr 17, 2018 at 4:45 PM, nicolas de loof
<nicolas...@gmail.com> wrote:
> Job class hierarchy is full of hand written JSON parsing

I suspect such cases are fixable, which would take some work, but on
the other hand we would get a clearer code base as a result anyway.

Sure, but this will be for future version then

We want JCasC to support current releases of Jenkins by Praqma customers (and others), not require bleeding edge Jenkins core.

 

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

[Liam Newman]

Ewelina, Nicolas,

I'm jumping in because I don't see anyone mentioning who will be doing this.

As JEP-1 sponsor, I would like to remind you that part of your duties as sponsors of JEP-201 include different viewpoints and design suggestions in your JEP. (I'm jumping in because I don't see anyone mentioning who will be doing this.)  Even if you choose not to use the suggestions, they need to be represented in the "Reasoning" section.

Just as Jesse pulled this feedback to a discussion here so it wouldn't be lost in IRC, it will need to be distilled from this extended discussion to be added to the JEP.  Here's two examples of "Reasoning" sections with significant content:

https://github.com/jenkinsci/jep/tree/master/jep/1#reasoning

https://github.com/jenkinsci/jep/tree/master/jep/200#reasoning

It looks to me like there are at least three threads here: automatic symbol inference, YAML Map syntax and Credentials, and Replacing  Job DSL Groovy syntax. There might also be a general list of features that have been deemed out-of-scope for the current release. 

You may ask Jesse if he'd be willing to submit PRs (adding to "Specification" or "Reasoning"), but ultimately it is your responsibility to make sure it happens. 

Thanks, 

-L. 

[Ewelina Wilkosz]

Thanks for keeping your eye on it Liam, I will take necessary actions this week!

[Ewelina Wilkosz]

Jesse, Nicolas, I'd like to follow Liam's suggestion about updating JEP-201's Reasoning section

Any of you interested in contributing to that one? Please let me know, so we don't duplicate :) I hope to have something around Friday

On Friday, April 20, 2018 at 8:26:45 PM UTC+2, Liam Newman wrote:

[Jesse Glick]

On Wed, May 2, 2018 at 3:58 PM, Ewelina Wilkosz <ewel...@gmail.com> wrote:
> Any of you interested in contributing to that one? Please let me know, so we
> don't duplicate

I will leave the reasoning to you! Just bringing up things that seemed
to merit discussion.

[Ewelina Wilkosz]

just a short update - JEP update in progress, next week we're planning a short maintainers status meeting where I hope to clarify stuff and PR will be sent

[Liam Newman]

Just FYI: Nicolas asked a question of me in a GH comment:

https://github.com/jenkinsci/jep/commit/78bede6c731e7a85a90ca374ca1c394e219aa961#commitcomment-28894501

@bitwiseman Discussion on plugin management was probably introduced in this JEP too early as we don't have a prototype for it (update center doesn't even have the required metadata)
Would it make sense to revert this commit so we can follow-up with JCasC as an approved JEP, and prepare another JEP about plugin management (which would apply to docker images and probably few other use-cases) ?

I figured I'd respond here rather than buried in GH comments. 

Nicolas, 

To be clear:  I'm not Reviewer for this JEP.   I set it back to this "Draft" with the agreement of the sponsor as part of a clarification of the JEP process.  

It sounds like Ewelina is working on further updates (including incorporating the JOM feedback - yay!) and hasn't requested Review for Acceptance yet.  As sponsor, when she believes the JEP meets the requirements for Accepted status, I'm sure she'll request that it be reviewed again. 

Ewelina, 

Thanks for taking the time to incorporate feedback and to make sure everyone is clear about the on-going status of the project.  I don't know that I need to be involved in the maintainers meeting, but feel free to send me an invite. 

-Liam

[nicolas de loof]

2018-05-08 21:08 GMT+02:00 Liam Newman <bitwi...@gmail.com>:

Just FYI: Nicolas asked a question of me in a GH comment:

https://github.com/jenkinsci/jep/commit/78bede6c731e7a85a90ca374ca1c394e219aa961#commitcomment-28894501

@bitwiseman Discussion on plugin management was probably introduced in this JEP too early as we don't have a prototype for it (update center doesn't even have the required metadata)
Would it make sense to revert this commit so we can follow-up with JCasC as an approved JEP, and prepare another JEP about plugin management (which would apply to docker images and probably few other use-cases) ?

I figured I'd respond here rather than buried in GH comments. 

Nicolas, 

To be clear:  I'm not Reviewer for this JEP.   I set it back to this "Draft" with the agreement of the sponsor as part of a clarification of the JEP process.  

Yes, that's why I asked you to revert this if we just want to get this discussion in a separate document.

 

It sounds like Ewelina is working on further updates (including incorporating the JOM feedback - yay!) and hasn't requested Review for Acceptance yet.  As sponsor, when she believes the JEP meets the requirements for Accepted status, I'm sure she'll request that it be reviewed again. 

ok, make sense as well.

 

Ewelina, 

Thanks for taking the time to incorporate feedback and to make sure everyone is clear about the on-going status of the project.  I don't know that I need to be involved in the maintainers meeting, but feel free to send me an invite. 

-Liam

On Tuesday, May 8, 2018 at 3:34:28 AM UTC-7, Ewelina Wilkosz wrote:

just a short update - JEP update in progress, next week we're planning a short maintainers status meeting where I hope to clarify stuff and PR will be sent

On Thursday, May 3, 2018 at 12:24:07 AM UTC+2, Jesse Glick wrote:

On Wed, May 2, 2018 at 3:58 PM, Ewelina Wilkosz <ewel...@gmail.com> wrote:
> Any of you interested in contributing to that one? Please let me know, so we
> don't duplicate

I will leave the reasoning to you! Just bringing up things that seemed
to merit discussion.

--

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/56c5a171-f7ed-4f53-bd67-c79bb182bb91%40googlegroups.com.

[nicolas de loof]

> The credentials domain syntax, IIRC something like

system:
  ? # global
  : -credentials
    # …

baffles everyone who sees it. Just because internally `credentials`
uses a `Map` from (I guess) domains to lists of credentials does not
mean you must reflect that in YAML syntax. You should rather model a
list of credentials, each of which may have an optional attribute for
the domain (ID?).

We received comparable feedback many times, it seems for most people yaml is json without quotes, and more complex notations just confuse the reader.

In the meantime for internal design reasons we introduced a model for the parsed yaml tree, and as a result of this feedback this model as been limited to only support simplest structures : Lists and maps, with only support for strings as key.

As a result, here is how a global username/password system credential entry looks like :

credentials:
  system:
    domainCredentials:
      - credentials:
          - usernamePassword:
              scope:    SYSTEM
              id:       sudo_password
              username: root
              password: ${SUDO_PASSWORD}

> Using Job DSL Groovy to create jobs is fine as a short-term workaround
but should not be how `TopLevelItem`s are defined in 1.0 I think.

Alternate solution will require some significant changes in core (Job hierarchy highly relies on hand-written json parsing)

For 1.0 my plan is to propose this job-dsl configuration support to be owned by job-dsl plugin itself, so users who want to rely on it can, and this doesn't close the door to an alternate approach in (near?) future.