Check casc version to match changes in YamlSource

[madsjakobsen]

Casc version 1.41 introduced changed to YamlSource which breaks the current implementation in the operator. The commit checks the version of casc and loads the yaml accordingly.

Changes

Check casc plugin version, to accommodate for api changes in version casc v1.41 in YamlSource

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

• Includes tests (if functionality changed/added)
• Includes docs (if user facing)
• Commit messages follow commit message best practices

See the contribution guide for more details.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS.

[jetersen]

This can easily be reduced to two lines.
Though I do not appreciate it calling a restricted method.

[jetersen]

Here is a clearer version that is backwards compatible.

Suggested change

	def source = new io.jenkins.plugins.casc.yaml.YamlSource(stream, io.jenkins.plugins.casc.yaml.YamlSource.READ_FROM_INPUTSTREAM)
	def plugin = jenkins.model.Jenkins.instance.getPluginManager().whichPlugin(io.jenkins.plugins.casc.ConfigurationAsCode)
	def version = plugin.getVersionNumber()
	def source
	switch (version) {
	case { it.isNewerThanOrEqualTo(new VersionNumber("1.41")) }:
	source = new io.jenkins.plugins.casc.yaml.YamlSource(stream)
	break
	default:
	source = new io.jenkins.plugins.casc.yaml.YamlSource(stream, io.jenkins.plugins.casc.yaml.YamlSource.READ_FROM_INPUTSTREAM)
	break
	}
	io.jenkins.plugins.casc.ConfigurationAsCode.get().configureWith(source)
	def source = io.jenkins.plugins.casc.yaml.YamlSource.of(stream)
	io.jenkins.plugins.casc.ConfigurationAsCode.get().configureWith(source)

However this method is marked restricted for a reason. https://github.com/jenkinsci/configuration-as-code-plugin/blob/1c32633d879379b83bb4c0475bf9876c8f88c051/plugin/src/main/java/io/jenkins/plugins/casc/ConfigurationAsCode.java#L606-L611

If you need an official way to call with stdin we currently offer the ApplyConfigurationCommand That will takes stdin.

If you need a better entrypoint you could have asked for it when creating this.

[jetersen]

As the one who made the so called breaking change.
I do not think so.

You were calling the wrong API in the first place.
This was the very reason why we offer the YamlSource.of() method, so that the internal method could change.

The constructor for YamlSource class was changed and we removed the YamlReader functional interface because it caused leaking readers that were holding file handles open when using Path input source.

JCasC still supports InputStream conversion:
https://github.com/jenkinsci/configuration-as-code-plugin/blob/1c32633d879379b83bb4c0475bf9876c8f88c051/plugin/src/main/java/io/jenkins/plugins/casc/yaml/YamlUtils.java#L83-L96

See this PR: jenkinsci/configuration-as-code-plugin#1398

And see specifially this comment: jenkinsci/configuration-as-code-plugin#1398 (comment)

[madsjakobsen]

@jetersen Thanks for the detailed explanation! And sorry for not reaching out before creating the PR, my apologies for rushing it, I should have done my due diligence.

The two lines are certainly nicer than what I ended up with😰 I do understand your concern with calling configureWith. I am not familiar enough with the operator and jcasc code base to suggest an alternative to the original implementation, so this issue is better handled by someone else than me.

I am very green when it comes to contributing to any project, so I am unsure what a good follow up would be, my suggest is to close this PR and open an issue regarding how the configurations is applied, does that sound like the right next step to you?

[jetersen]

I think the next should be applying my suggestions. Than the maintainers can merge this PR.
Credit where credit is due. I am very thankful that you found this issue.

Than we can consider our next step in another issue or PR.

[madsjakobsen]

👍 I applied you suggestion, and thanks again for your feedback. I am just excited to start using anchors in my configuration😀

[jetersen]

Suggested change

	String[] configContent = ['''%s''']
	import hudson.util.VersionNumber
	String[] configContent = ['''%s''']
	String[] configContent = ['''%s''']

[jetersen]

@madsjakobsen Thanks for poking us over on gitter. Much appreciated
I would say your better off poking me here on GitHub.

Going forward I would like to more involved in this project. Especially on any changes to jcasc.
Perhaps setting up code owners might be useful: https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners
@jkhelil @tomaszsek how does that sound?

[tomaszsek]

@jetersen Regarding code-owners first we have to separate code to introduce it.

[tomaszsek]

@madsjakobsen Thanks for the PR 🎉

[jtgans]

@tomaszsek Any chance we can get an emergency point release with this fix? There are a number of plugins that need updating that depend on 1.41 of JCasC.

[jetersen]

Users are still waiting for a patch.

Perhaps we could get a v0.4.1 with this PR? This changeset is backwards compatible with all JCasC versions.

[sknight80]

Hi, What is the deal with the v0.4.1? I can not use the JCasC implementation in our new Jenkins Operator setup and I am seeing multiple groovy scripts (provided by our configuration) ran multiple times.