kuleuven / jenkins-mattermost-plugin

Jenkins plugin for Mattermost
MIT License
24 stars 46 forks source link

Change of endpoint from String to Secret incomplete? #38

Closed riy closed 4 years ago

riy commented 4 years ago

Hey there,

Thanks for the great work on the plugin. It's working perfectly fine for us so far, but the latest release 2.7.1 doesn't seem to work as expected.

In the commit diff I see that that endpoint type has been changed from String to hudson.util.Secret. So in our JobDSL script this doesn't work anymore:

mattermostNotifier {
  endpoint('https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg')
  buildServerUrl('https://jenkins.mycompany.com/')
  room('#build-notifications')
  startNotification(false)
  notifyAborted(true)
  notifyFailure(true)
  notifyNotBuilt(false)

I'm not an expert on Jenkins, but I assume that the endpoint as a Secret now refers to a secret that I need to set up manually at https://jenkins.mycompany.com/credentials/store/system/domain/_/newCredentials.

Usually such a field would show up as a dropdown form field in a job's configuration, with the option to pick a secret that was specified by the Jenkins admin at the aforementioned URL. But in this case it's still a regular textfield. Also, in the config.jelly I still see it configured as:

        <f:entry title="Endpoint" help="/plugin/mattermost/help-projectConfig-mattermostEndpoint.html">
            <f:textbox field="endpoint" />
        </f:entry>

Am I interpreting this correctly? Any help is appreciated.

Thanks!

jovandeginste commented 4 years ago

Ref: Jenkins CI Jira

jovandeginste commented 4 years ago

@riy would you be able to test a version that I built? I don't use the jobDSL plugin myself...

riy commented 4 years ago

@jovandeginste Sure thing. I'll be back at the office on Monday. Send over what you have and I'll give it a try.

jovandeginste commented 4 years ago

@riy I'm still working on it, the tests keep failing now :)

steve-todorov commented 4 years ago

Hi there,

I'm experiencing the same issue with Jenkins 2.205. I'll be more than happy to checkout a test build when it becomes available.

riy commented 4 years ago

@riy I'm still working on it, the tests keep failing now :)

Alright, ready when you are :-)

covert8 commented 4 years ago

mattermost.zip @riy We have done some major refactoring in the code. Could you go ahead and test this one? PS changes will be pushed to master but are already on dev branch

riy commented 4 years ago

@covert8 It's not working. I tried the same configuration as noted above. No error message is show, but there's also no message posted into the channel. I also tried "Test Connection" and it just says "Failure" with no further message or log output.

covert8 commented 4 years ago

@riy I've re-read your question and i think it is important to clarify the following:

Could you please confirm all the above is true in your setup? Otherwise I'm understanding the question wrongly.

riy commented 4 years ago

@covert8 Yes, all three of the above is what is configured: no credentials used, endpoint url in plain text etc..

I just saw that the endpoint has changed from String to hudson.util.Secret, hence my assumption that I'd have to set up credentials since v2.7.1 (but I didn't, I just installed the fix release and ran it with my job unchanged).

So: the tests I ran earlier today with your fix release use the exact same configuration as what I posted in my initial message in this issue:

mattermostNotifier {
  endpoint('https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg')
  buildServerUrl('https://jenkins.mycompany.com/')
  room('#build-notifications')
  startNotification(false)
  notifyAborted(true)
  notifyFailure(true)
  notifyNotBuilt(false)

The fix release 2.8.0-RC (and 2.7.1) still doesn't work with this snippet. Version 2.7.0 works, so I rolled the plugin back to that version.

riy commented 4 years ago

I don't know if here the constructor is called or if this setter is called to set the endpoint. If the setter is called then your fix should work. But when I look at my snippet I see:

mattermostNotifier {
  endpoint('https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg')
  [...]

... which looks to me like the constructor is called. In this case the endpoint in this call contains a String, and not a hudson.util.Secret, so the call directly against the constructor would fail.

And as you can see here this attribute used to be a String before, in v2.7.0. The change to hudson.util.Secret is what I believe makes this fail now.

jovandeginste commented 4 years ago

@riy do you see any stack traces related to mattermost in your Jenkins log to help us confirm/pinpoint?

riy commented 4 years ago

I took another look. The log I looked into was overwritten, hence I missed the actual error message. I was able to reproduce the issue, and maybe figure out why it currently fails. So when I click "Test Connection" I see this in the logs:

Error posting to Mattermost
java.net.MalformedURLException: unknown protocol: proxyserver.mycompany.com
    at java.net.URL.<init>(URL.java:607)
    at java.net.URL.<init>(URL.java:497)
    at java.net.URL.<init>(URL.java:446)
    at jenkins.plugins.mattermost.StandardMattermostService.setupProxy(StandardMattermostService.java:201)
    at jenkins.plugins.mattermost.StandardMattermostService.publish(StandardMattermostService.java:141)
    at jenkins.plugins.mattermost.StandardMattermostService.publish(StandardMattermostService.java:116)
    at jenkins.plugins.mattermost.MattermostNotifier$DescriptorImpl.doTestConnection(MattermostNotifier.java:489)
    at java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:627)
    at org.kohsuke.stapler.Function$MethodFunction.invoke(Function.java:396)
    at org.kohsuke.stapler.Function$InstanceFunction.invoke(Function.java:408)
    at org.kohsuke.stapler.Function.bindAndInvoke(Function.java:212)
    at org.kohsuke.stapler.SelectionInterceptedFunction$Adapter.invoke(SelectionInterceptedFunction.java:36)
    at org.kohsuke.stapler.verb.HttpVerbInterceptor.invoke(HttpVerbInterceptor.java:48)
    at org.kohsuke.stapler.SelectionInterceptedFunction.bindAndInvoke(SelectionInterceptedFunction.java:26)
    at org.kohsuke.stapler.Function.bindAndInvokeAndServeResponse(Function.java:145)
    at org.kohsuke.stapler.MetaClass$11.doDispatch(MetaClass.java:535)

When I remove the proxy server from the plugin settings it works. So maybe this is another issue we now have, because we do need the proxy server settings to be there.

And/but: when I use the JobDSL plugin it still fails when building my scripts. This is what I get, same as before:

ERROR: (feature_branch_initial_jobs.groovy, line 54) No signature of method: javaposse.jobdsl.plugin.structs.DescribableContext.endpoint() is applicable for argument types: (java.lang.String) values: [https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg]
Possible solutions: print(java.io.PrintWriter), print(java.lang.Object)
jovandeginste commented 4 years ago

Ok thanks! We were aware that the proxy part was at risk, since the overhaul changed so many things, but had not taken the time yet to write the tests.

Maybe the stack trace will help my colleague to pinpoint the way it should work. It does sound like a missing extra constructor to convert strings to secrets...

covert8 commented 4 years ago

Hi @riy, there was unexpected behaviour in the passing of the proxy address (expecting http/s). This has been reworked. Would you kindly try again, bearing in mind you might need to revert again. In summary: (nonauth) proxy behavior has been reworked and tested. mattermost.zip

riy commented 4 years ago

Hey Louis,

I just tested it. It's still failing with

ERROR: (feature_branch_initial_jobs.groovy, line 54) No signature of method: javaposse.jobdsl.plugin.structs.DescribableContext.endpoint() is applicable for argument types: (java.lang.String) values: [https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg]
Possible solutions: print(java.io.PrintWriter), print(java.lang.Object)

The plugin version is still shown as 2.8.0-RC2. This seems to be the same version as yesterday?

jovandeginste commented 4 years ago

@riy @covert8 was talking about the proxy part... the dsl part is still WIP

riy commented 4 years ago

@jovandeginste Sorry, I missed that. Yes, the proxy stuff works now :-)

covert8 commented 4 years ago

Hi @riy, I'm trying to replicate the problem. I think i may have found the solution. But could you send me a jobdsl which uses the mattermost plugin? This is what I have currently. JenkinsJobManagement jobManagement = new JenkinsJobManagement(System.out, Collections.emptyMap(), new File(".")); DslScriptLoader dslScriptLoader = new DslScriptLoader(jobManagement); dslScriptLoader.runScript("def project = 'test'" + "job('test') {" + "mattermostNotifier {\n" + " endpoint('http://localhost:"+testListener.port+"')\n" + " room('#build-notifications')\n" + " startNotification(false)\n" + " notifyAborted(true)\n" + " notifyFailure(true)\n" + " notifyNotBuilt(false)" + "}}"); Assert.assertTrue(testListener.messages.size() > 0);

riy commented 4 years ago

I've simplified the one I'm testing with down to this for you:

job('my-mattermost-job') {
    description("Testing the new Mattermost plugin")

    wrappers {
        timestamps()
        colorizeOutput()
    }

    publishers {
        mattermostNotifier {
            endpoint('https://mattermost.mycompany.com/hooks/gtw8oerghvk6lpoqwrd5fg12hg')
            buildServerUrl('https://jenkins.mycompany.com/')
            room('#mattermost')
            startNotification(true)
            notifyAborted(true)
            notifyFailure(true)
            notifyNotBuilt(true)
            notifySuccess(true)
            notifyUnstable(true)
            notifyBackToNormal(true)
            notifyRepeatedFailure(true)
            includeTestSummary(true)
            includeCustomMessage(true)
        }
    }
}

The settings in https://jenkins.mycompany.com/configure look like this:

grafik

covert8 commented 4 years ago

Hi @riy , we ran a dsl job test and encountered the same issue. It is supposedly fixed. 3 times the charm. mattermost.zip

jovandeginste commented 4 years ago

@riy Please let us know your findings with the most recent zip flie!

riy commented 4 years ago

Good morning, @covert8 and @jovandeginste,

Yes, it works! Everything is fine and works as it should. Good job :-)

jovandeginste commented 4 years ago

That is groovy! I will cut a new release shortly. Will make it a 3.0.

Did you loose configuration when upgrading?

riy commented 4 years ago

Alright, I'll upgrade then and test it once more.

No, I didn't see any configuration getting lost. But this is because we have a job that creates/configures all of our jobs. Even if something was lost I wouldn't notice due to the automatism of recreating all jobs from scratch on each push.

jovandeginste commented 4 years ago

After the "test" and "package" steps in maven came the "findbugs". Still trying to solve the last one ...

jovandeginste commented 4 years ago

@riy @covert8 the new release is launched - I hope no new regressions appear!

Thanks for your help and your patience.

riy commented 4 years ago

You're welcome! Thank you for the quick turnarounds and the fix!

riy commented 4 years ago

All is well, the 3.0.0 release works fine for us 👍