search

jenkinsci / ivy-plugin

Jenkins ivy plugin
https://plugins.jenkins.io/ivy/
MIT License
16 stars 42 forks source link

[JENKINS-53570] JCasC Compatibility #31

Closed darxriggs closed 5 years ago

darxriggs commented 5 years ago

This is still work in progress... I have to adapt all the data-binding to make the UI work (again).

darxriggs commented 5 years ago

So far the exported config just contains the configurations property.

Example:

hudson.ivy.IvyBuildTrigger.xml

<?xml version='1.0' encoding='UTF-8'?>
<hudson.ivy.IvyBuildTrigger_-DescriptorImpl plugin="ivy@1.28">
  <configurations>
    <hudson.ivy.IvyBuildTrigger_-IvyConfiguration>
      <name>setting1</name>
      <ivyConfPath>file1.xml</ivyConfPath>
    </hudson.ivy.IvyBuildTrigger_-IvyConfiguration>
    <hudson.ivy.IvyBuildTrigger_-IvyConfiguration>
      <name>setting2</name>
      <ivyConfPath>file2.xml</ivyConfPath>
    </hudson.ivy.IvyBuildTrigger_-IvyConfiguration>
  </configurations>
  <extendedVersionMatching>true</extendedVersionMatching>
</hudson.ivy.IvyBuildTrigger_-DescriptorImpl>

exported configuration

unclassified:
  ivyBuildTrigger:
    configurations:
    - ivyConfPath: "file1.xml"
      name: "setting1"
    - ivyConfPath: "file2.xml"
      name: "setting2"

The extendedVersionMatching property is contained in the generated documenation and JSON schema but is not exported. Having invested some decent amount of time, I still don't understand why it is missing.

@casz / @ndeloof can you give me some hint want I am missing?

jetersen commented 5 years ago

@darxriggs for what reason are the fields marked volatile, perhaps it has something to do with it?

darxriggs commented 5 years ago

@casz I already tried with removed volatile and @CopyOnWrite. There is no effect on the exported config.

darxriggs commented 5 years ago

@casz / @ndeloof I have found the cause - see my comment here https://github.com/jenkinsci/configuration-as-code-plugin/commit/c9faa3119bab5e9b02e575e1e65f56051c1e9b4f#r32624910.

If I comment out if (attribute.equals(instance, reference)) continue; in BaseConfigurator then extendedVersionMatching is also exported.

Any comments on this?

darxriggs commented 5 years ago

Import Configuring the plugin via JCasC is now working correctly.

Export Exporting the config to YAML doesn't work 100% (not all properties are exported). This is due to the load() method used in constructors in combination with how JCasC is implemented. This could also be fixed using https://javadoc.jenkins.io/hudson/model/PersistentDescriptor.html. For the moment I decided against it, as it would require at least Jenkins 2.140.

jetersen commented 5 years ago

@darxriggs could you not just implement PersistentDescriptor? and add remove once +2.140?

https://github.com/jenkinsci/jenkins/blob/729303a6053ce7323b9fc4ec9ced54a86fc998e8/core/src/main/java/hudson/model/PersistentDescriptor.java

darxriggs commented 5 years ago

@casz Just implementing PersistentDescriptor is not sufficient. The load() method is annotated with @PostConstruct. This annotation is only evaluated on extensions with Jenkins 2.140 - see https://github.com/jenkinsci/jenkins/commit/c3988aae8ad66fde529830e799fe3d49cd241ebb. Therefore when I would remove calling the load() method in the constructor the configuration would not be loaded from disk on old Jenkins installations.

ndeloof commented 5 years ago

PersistentDescriptor has been introduced in 2.140 so that this behaviour change is homogeneous. Just update minimal required version

darxriggs commented 5 years ago

@ndeloof Exactly. Thanks for the info. We were just discussing the case when the minimum required Jenkins version is not yet upgraded to Jenkins 2.140.

@arothian Are you ok with upgrading the minimum required Jenkins version to 2.140? Then I would prepare an additional commit so that exporting the configuration with JCasC would be correct too.

arothian commented 5 years ago

Fine by me

jetersen commented 5 years ago

Consider targeting an LTS release 😄

darxriggs commented 5 years ago

@casz Thanks for the hint. I have followed this procedure in the past for some other plugins. But I am more and more of the opinion that just the minimal required version should be used and that a new plugin version should not force to user to upgrade to an LTS release.

darxriggs commented 5 years ago

@arothian Now this pull request is ready for review and merge.

darxriggs commented 5 years ago

@arothian can you provide an estimate when this change can be released via a new plugin version?

arothian commented 5 years ago

Given this update and the other jenkins version requirements updates, I'll drop a 2.0 release with these changes within the next few days.

darxriggs commented 5 years ago

much appreciated