-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JEP Submission: Jenkins Configuration as Code #31
Conversation
jep/0000/README.adoc
Outdated
| Jenkins Configuration as Code | ||
|
||
| Sponsor | ||
| https://github.com/praqma[Praqma] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have mentions of particular representatives on GitHub. There is no way to mention an org in the PR discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndeloof
You are welcome to mention the github org that the sponsor is a member of but that org can't be the sponsor.
See the definition and duties of a sponsor. The sponsor(s) must be a person able to be the point of contact.
jep/0000/README.adoc
Outdated
@@ -0,0 +1,238 @@ | |||
= JEP-0000: Jenkins Configuration as Code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it was discussed before, "Configuration as Code" is a bit ambiguous. When I was talking about use-cases of this plugin, @rtyler was proposing to rather use a more classic Infrastructure-as-Code term
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to disagree. "infrastructure" would make me thing we manage machines and OS, which isn't the case : we manage jenkins configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love either term, but "Configuration as Code" is better than "Infrastructure as Code", IMO. The latter feels like Puppet more than "Managing Jenkins global config and the like".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndeloof
The title may need to change later but does not block approval as draft.
jep/0000/README.adoc
Outdated
+DataBoundSetters+ & +DataBoundConstructors+ offer a natural way to construct Jenkins components from a set of key=value pairs. Most | ||
jenkins component (*) do rely on them and as such offer a 1:1 match between internal data structure and web UI configuration forms. | ||
|
||
(*) We noticed many +Descriptor+s do rely on manual parsing of +JSONObject+. We will need to fix them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to fix them independently of this JEP. The problem is that there are complex structures and custom forms sometimes. Would be great to know a list of plugins and core changes required for MVP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need to fix them
This seems to be a massive change -- I wouldn't be surprised if this affected hundreds of plugins, not to mention any plugins that would be newly incompatible since newInstance
isn't final
(and probably never will be).
I would strongly suggest looking into a way to get this implemented without requiring all @DataboundWhatever
for better compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the plugin offers extension points for such custom cases, it would be already helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if an extension point is introduce and plugins have to adopt them, we are back in the issue to require hundreds plugins to be fixed. @DataboundSetter
should be enough. Main issue AFAICT is one can't reset to null a Databound attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are back in the issue to require hundreds plugins to be fixed
I recommend you look into how many plugins use non-databound form submission data. I don't see how requiring moving to @DataboundX
is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DataboundSetter
is obvious way to document attributes and types for form submission. With custom databinding we can't guess this model, and generate a configuration-as-code schema/doc.
I've started reviewing existing Descriptors and indeed there's various patterns. optional blocks typically introduce a boolean pseudo-attribute and nested JSON document, while Descriptor use nullable attributes. This hardly can be translated into a @DataBoundSetter
(won't be invoked on null value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not both? Supporting pure DataBound plugins by a generic solution is perfectly fine. Regarding else, there should extension points which allow doing it without reworking to DataBounds. I think that the latter thing is inevitable, migration to databound forms may be even impossible in some cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prototype already defines a Configurator
extension point plugins can implement. But then they'll depend on this plugin. Issue I can see doing this is plugins will need to implement configuration twice : one for c-as-c and the other for web UI binding. Using DataBound mechanism ensure we are un sync.
I wonder what would prevent use of databound. After all it's just a replacement from foo = json.getXX()
to @DataBoundSetter setFoo(xx)
. If the raw form data is then computed to something else, same code can move into such a setter.
@oleg-nenashev @daniel-beck If you feel the prototype is not sufficient to begin discussion and consensus building. If you disagree with the design of the prototype, that should be discussed after the draft is accepted. See the submission conformance check section of JEP-1. And if that section is not sufficiently clear, please PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JEP-1 is accepted, so now I am going to request changes. JEP Sponsor should be a human being with GitHub account
@ndeloof Not to be picky, but would you mind not including the logo at this time? |
@bitwiseman asked me to dismiss the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted, there needs to be one or more people as sponsor, not an organization.
jep/0000/README.adoc
Outdated
// | ||
// Uncomment if there will be a BDFL delegate for this JEP. | ||
| BDFL-Delegate | ||
| https://github.com/ewelinawilkosz2[Ewelina Wilkosz] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ndeloof
Is there an email on the dev list or a comment in that JIRA naming this person as BDFL delegate (and getting agreement from other contributors)? If not, would you mind if I return this to BDFL until we have that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, no worries
They may be re-added later.
Approved as Draft JEP: 201. |
No description provided.