JENKINS-55920: add CasC support to GitHubPluginConfig. by dgarzon · Pull Request #205 · jenkinsci/github-plugin

dgarzon · 2019-02-01T19:29:39Z

This PR addresses the issue raised in the configuration-as-code-plugin: jenkinsci/configuration-as-code-plugin#672.

This change is

oleg-nenashev · 2019-02-01T19:34:03Z

Would be nice to have a JIRA ticket for it so that we add the case to the compatibility dashboard

On Fri, Feb 1, 2019, 20:29 Daniel Garzon ***@***.*** wrote: This PR addresses the issue raised in the configuration-as-code-plugin: jenkinsci/configuration-as-code-plugin#672 <jenkinsci/configuration-as-code-plugin#672>. ------------------------------ You can view, comment on, or merge this pull request online at: #205 Commit Summary - HOTFIX: add CasC support to GitHubPluginConfig. File Changes - *M* src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java <https://github.com/jenkinsci/github-plugin/pull/205/files#diff-0> (19) - *M* src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java <https://github.com/jenkinsci/github-plugin/pull/205/files#diff-1> (8) Patch Links: - https://github.com/jenkinsci/github-plugin/pull/205.patch - https://github.com/jenkinsci/github-plugin/pull/205.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#205>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoINzr9FtwlXmUW_z117HsEDebdT6ks5vJJWlgaJpZM4ae8P3> .

dgarzon · 2019-02-01T19:39:17Z

@oleg-nenashev here you go: https://issues.jenkins-ci.org/browse/JENKINS-55920

oleg-nenashev · 2019-02-01T19:58:59Z

Thank you!

…

On Fri, Feb 1, 2019, 20:39 Daniel Garzon ***@***.*** wrote: @oleg-nenashev <https://github.com/oleg-nenashev> here you go: https://issues.jenkins-ci.org/browse/JENKINS-55920 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#205 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoKVyEKZJfDr7oW5Y16LC938QsnEJks5vJJfpgaJpZM4ae8P3> .

oleg-nenashev

Binary compat issue needs to be fixed before the merge

dgarzon · 2019-02-01T20:47:11Z

@oleg-nenashev I tested the hpi and keeping the old setter does not work, as it is picked up by
CasC even though it is not annotated. Any ideas why?

dgarzon · 2019-02-01T20:52:24Z

@oleg-nenashev actually, it does work. The only thing we needed was annotating setOverrideHookUrl with @DataBoundSetter and exposing a getter. Now CasC works as expected 👍

oleg-nenashev

Looks good

KostyaSha · 2019-02-02T17:15:36Z


+    @DataBoundConstructor
    public GitHubPluginConfig() {
        load();


DataBoundConstructor is used for binding new data, load() is loading old data. Thats looks wrong

This is the constructor being used by CasC, and it uses the getter and setter to mutate the instance. The other constructor would make List<GitHubServerConfig> configs required, which should not. What do you suggest would be an appropriate fix to address your concerns?

@jglick could you describe why DBC is set on top of Bean constructor that loads data?

This is wrong; the annotation should be deleted. GitHubPluginConfig is a singleton (GlobalConfiguration) so it may have @DataBoundSetters but that is it.

jglick · 2019-02-06T18:42:58Z


+    @DataBoundConstructor
    public GitHubPluginConfig() {
        load();


This is wrong; the annotation should be deleted. GitHubPluginConfig is a singleton (GlobalConfiguration) so it may have @DataBoundSetters but that is it.

jglick · 2019-02-06T18:45:50Z

BTW I strongly recommend inclusion of a functional test in PRs like this. There are several examples to look at of plugins which have a test dep on configuration-as-code for this purpose.

KostyaSha

resolve @jglick comments

er1c · 2019-03-01T16:28:59Z

I just ran into this issue, are there still outstanding issues I could help with to get it merged?

er1c · 2019-03-01T20:58:44Z

I have a unit test written for this, but I'm getting a bunch of unrelated errors doing mvn test:

shouldNotAllowSSRFUsingHookUrl(org.jenkinsci.plugins.github.config.GitHubPluginConfigTest)  Time elapsed: 0.159 sec  <<< ERROR!
org.jvnet.hudson.reactor.ReactorException: java.lang.Error: java.lang.reflect.InvocationTargetException
Caused by: java.lang.Error: java.lang.reflect.InvocationTargetException
Caused by: java.lang.reflect.InvocationTargetException
Caused by: java.lang.NullPointerException

I made sure I'm running java8, what's the proper procedure here?

Thanks,

Eric

jglick · 2019-03-01T21:35:42Z

I do not get such an error. Hard to guess at the cause without seeing the stack trace of the NullPointerException.

er1c · 2019-03-02T02:07:04Z

It looks like these tests are already marked as "existing failures" on the master branch: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fgithub-plugin/detail/PR-208/1/tests, so the CI is having issues too

er1c · 2019-03-02T06:01:03Z

Here is the full (example) error I get: https://gist.github.com/er1c/9713ccac7e6b6ece4516b9dac9f9e20a . I tried on both my OSX and a linux box, same result.

I just checked out master and it does not have this error, so let me see if I can figure out what in this PR broke the build.

jetersen · 2019-03-02T06:46:39Z

BTW I strongly recommend inclusion of a functional test in PRs like this. There are several examples to look at of plugins which have a test dep on configuration-as-code for this purpose.

This might not be as easy as suggested. JCasC requires that the plugin targets Java 8 and at least 2.60.3 Jenkins core version 😢

Unless you are fine with bumping those values.

Some quick number crunching on http://stats.jenkins.io/pluginversions/github.html

92.88% are above 2.60 who have this plugin installed

er1c · 2019-03-02T19:34:13Z

I'm going to submit a separate PR that upgrades github-plugin to jekins.version 2.60.3 & java8. I have it compiling, and most of the tests successful, just working through other code changes that would be required for the updated dependencies. That should unblock this PR to include a JCasC test.

er1c · 2019-04-21T18:46:44Z

I would suggest closing this PR and referencing: #210 for ongoing tracking.

dgarzon changed the title ~~HOTFIX: add CasC support to GitHubPluginConfig.~~ JENKINS-55920: add CasC support to GitHubPluginConfig. Feb 1, 2019

JENKINS-55920: add CasC support to GitHubPluginConfig.

4bed1b8

dgarzon force-pushed the HOTFIX-add-casc-support branch from ed81fa2 to 4bed1b8 Compare February 1, 2019 19:39

oleg-nenashev requested changes Feb 1, 2019

View reviewed changes

Comment thread src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java Outdated

Comment thread src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java Outdated

dgarzon force-pushed the HOTFIX-add-casc-support branch from 9390573 to da5d737 Compare February 1, 2019 20:24

dgarzon force-pushed the HOTFIX-add-casc-support branch from da5d737 to 5a6ccd4 Compare February 1, 2019 20:52

JENKINS-55920: address PR comments.

9389d8b

dgarzon force-pushed the HOTFIX-add-casc-support branch from 5a6ccd4 to 9389d8b Compare February 1, 2019 20:56

dgarzon mentioned this pull request Feb 1, 2019

githubpluginconfig's hookUrl not working? jenkinsci/configuration-as-code-plugin#672

Closed

oleg-nenashev approved these changes Feb 1, 2019

View reviewed changes

KostyaSha reviewed Feb 2, 2019

View reviewed changes

jglick suggested changes Feb 6, 2019

View reviewed changes

KostyaSha requested changes Feb 14, 2019

View reviewed changes

Address PR comments

8f4a7e4

er1c mentioned this pull request Mar 1, 2019

JENKINS-55920: add CasC support to GitHubPluginConfig. - with test #208

Closed

er1c mentioned this pull request Mar 2, 2019

JENKINS-55920: add CasC support to GitHubPluginConfig w/Tests #210

Closed

dgarzon closed this Apr 23, 2019

dgarzon deleted the HOTFIX-add-casc-support branch April 23, 2019 00:47

jenkins-infra-bot mentioned this pull request Jan 24, 2022

[JENKINS-55920] CasC compatibility with GitHub Plugin #667

Closed

Uh oh!

Conversation

dgarzon commented Feb 1, 2019 • edited by KostyaSha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleg-nenashev commented Feb 1, 2019 via email

Uh oh!

dgarzon commented Feb 1, 2019

Uh oh!

oleg-nenashev commented Feb 1, 2019 via email

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dgarzon commented Feb 1, 2019

Uh oh!

dgarzon commented Feb 1, 2019

Uh oh!

oleg-nenashev left a comment

Choose a reason for hiding this comment

Uh oh!

KostyaSha Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

dgarzon Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

KostyaSha Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jglick Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jglick Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jglick commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KostyaSha left a comment

Choose a reason for hiding this comment

Uh oh!

er1c commented Mar 1, 2019

Uh oh!

er1c commented Mar 1, 2019

Uh oh!

jglick commented Mar 1, 2019

Uh oh!

er1c commented Mar 2, 2019

Uh oh!

er1c commented Mar 2, 2019

Uh oh!

jetersen commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

er1c commented Mar 2, 2019

Uh oh!

er1c commented Apr 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dgarzon commented Feb 1, 2019 •

edited by KostyaSha

Loading

jglick commented Feb 6, 2019 •

edited

Loading

jetersen commented Mar 2, 2019 •

edited

Loading