JENKINS-55920: add CasC support to GitHubPluginConfig w/Tests - #210
Conversation
bdfd613 to
6a56228
Compare
| <dependency> | ||
| <groupId>io.jenkins</groupId> | ||
| <artifactId>configuration-as-code</artifactId> | ||
| <version>1.0</version> |
There was a problem hiding this comment.
use the latest released version please (1.7) https://github.com/jenkinsci/configuration-as-code-plugin/releases
timja
left a comment
timja
left a comment
There was a problem hiding this comment.
Could you point the base branch to be your java 8 PR so the differences can be seen?
|
@er1c would be great if you could rebase: |
6a56228 to
ea585ca
Compare
jetersen
left a comment
jetersen
left a comment
There was a problem hiding this comment.
Please see the updated https://github.com/jenkinsci/configuration-as-code-plugin/blob/master/docs/PLUGINS.md#how-to-test
I suggest you add an export test as well 😄
ea585ca to
4c98e97
Compare
|
This has been rebased, and the outstanding review comments addressed. |
| @Test | ||
| @ConfiguredWithCode("configuration-as-code.yml") | ||
| public void export_configuration() throws Exception { | ||
| /* TODO (From JCASC): Need to provide some YAML assertion library so that the resulting exported yaml |
There was a problem hiding this comment.
We need to update the plugin docs but it is being tested in this way in a few plugins and in the configuration-as-code repo:
https://github.com/jenkinsci/azure-keyvault-plugin/blob/master/src/test/java/org/jenkinsci/plugins/azurekeyvaultplugin/ConfigAsCodeTest.java#L33
There was a problem hiding this comment.
| @@ -0,0 +1,17 @@ | |||
| unclassified: | |||
|
|
|||
| githubpluginconfig: | |||
There was a problem hiding this comment.
maybe another more minimal test which shows what you need if you set manage hooks false and no hook url?
jglick
left a comment
jglick
left a comment
There was a problem hiding this comment.
Removing my request for review as I work on neither this plugin nor configuration-as-code.
|
@er1c Thanks for the good work! Thanks |
| public void export_configuration() throws Exception { | ||
| /* TODO (From JCASC): Need to provide some YAML assertion library so that the resulting exported yaml | ||
| stream can be checked for expected content. */ | ||
| ConfigurationAsCode.get().export(System.out); |
There was a problem hiding this comment.
never user System.out, use loggers if you want to print something.
|
Hi, we're having this issue where |
|
I'd guess https://github.com/jenkinsci/github-plugin/pull/210/files#r277181851 is still a blocker, no? |
|
Any movement on this? My team is blocked by not being able to set the |
|
Basically the only thing to set manually at startup or after a Jenkins upgrade. I would be happy to help with this issue, I can do some testing but I am no Java/Groovy dev. |
|
Based on @er1c's Github contribution history, it looks like he probably doesn't have bandwidth to work on this anymore. Does anyone here have the Java experience to take over this PR? I am also hitting this particular issue. |
I'm currently slammed and don't have time for this. For anyone who is blocked, you can work around it via: https://gist.github.com/er1c/c057640d43c0dce149b0a0c23d1ce153#file-jenkins-github-yaml-L80 I think the remaining "todos" are straightforward, it mostly just needs someone to steward the PR. You probably don't need much java experience :) |
… simply deprecate the overrideHookUrl, and adding additional tests
4c98e97 to
b08bc56
Compare
|
Thanks @er1c for picking this up 👍 |
12 participants
This is based upon both #205 and #209
I can rebase the PR once #209 is addressed to make the PR cleaner.
This change is