jenkinsci / gogs-webhook-plugin

Jenkins Gogs Webhook
https://plugins.jenkins.io/gogs-webhook/
MIT License
79 stars 42 forks source link

Fix branches with slash #25

Closed pascalw closed 6 years ago

pascalw commented 7 years ago

20 was merged yesterday, supporting Multibranch projects which is great!

However for my use case it was not working because I typically prefix my branches with feature/ or bugfix/ etc. This was not supported by the code handling the webhook for two reasons:

  1. It derived the branch name from the Git ref by taking the part after the last / character. This means that refs/heads/feature/test-branch would be converted into test-branch where it should have been feature/test-branch.

  2. Jenkins internally URL encodes job names. In Multibranch projects job names are derived from the SCM branch names. This means that for a branch named feature/test-branch the corresponding Jenkins job will be named feature%252Ftest-branch and should be passed to jenkins.getItemByFullName in that form.

This PR fixes both of these problems and in the process cleaned up some duplication introduced by #20.

jmMeessen commented 7 years ago

Maybe we could work together @pascalw to add some Unit Test for that part of the code.

Adding Integration tests is still "Work In Progress"

sanderv32 commented 7 years ago

Can you check the conflicts which are introduced after merging PR #26 ?

pascalw commented 7 years ago

@sanderv32 yes I'll rebase later today or tomorrow.

@jmMeessen I only saw your PR after I made the changes in this PR. After I rebase I'll try to add a test for this fix. That will probably require me to mock a bunch more of the Jenkins API, but let's see how that goes. Having real integration tests would be nice indeed 👍

pascalw commented 7 years ago

@sanderv32 @jmMeessen during work on this I realised that the changes in #20 don't completely behave as expected.

The code now looks for a Job named jobName/branchName. However on the first push to a branch this job will not exist yet and thus no build will be scheduled. It seems to me that the webhook should not be looking at the branch specific jobs, but rather work with the WorkflowMultiBranchProject. A WorkflowMultiBranchProject can be scheduled to build, in which case it will perform a branch index, create jobs for branches etc. However I'm not very familiar with Jenkins so internals so I'm completely sure this is the right approach. What do you think?

jmMeessen commented 7 years ago

Interesting observation @pascalw . I am not familiar either with these two ways of triggering jobs or builds. While the proposal of the initial PR will answers a type of use case, the WorkflowMultiBranchProject is very popular. Many people are looking for this behavior.

I didn't look closely to the PR as I was concentrating on adding tests. I am now entering a month of heavy work so I will not have time to dig into the matter (and finish my integration test PR)

dduportal commented 7 years ago

Hello @pascalw , your description is correct: the webhooks shall trigger a "build" of the WorkflowMultiBranchProject project .

This exactly what the Github or BitBucket webhooks are doing in Jenkins :) => This is the goal to achieve.

Here is an example of "standard" configuration in Gogs: capture d ecran 2017-05-22 a 14 50 03

jmMeessen commented 7 years ago

@pascalw @sanderv32 I proposed my pull request #27 with my integration tests. There is only the simple case smoke test. I plan to add the integration test for pull request #20 and for this one as soon as possible.

sanderv32 commented 7 years ago

@jmMeessen Take your time. I rather release a quality version than a buggy version.

sanderv32 commented 7 years ago

Hi @jmMeessen , how is it going with the unit tests?

jmMeessen commented 7 years ago

My conference is now done. I am catching up with my (paid) work. I will start again on this project real soon now.

/- Jmm

Le lun. 12 juin 2017 à 13:02, Alexander notifications@github.com a écrit :

Hi @jmMeessen https://github.com/jmmeessen , how is it going with the unit tests?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/jenkinsci/gogs-webhook-plugin/pull/25#issuecomment-307758186, or mute the thread https://github.com/notifications/unsubscribe-auth/AFu5fTvLXDLTOQon9-KtVvGTJGFQocLDks5sDRqrgaJpZM4NcBft .

sanderv32 commented 6 years ago

@pascalw Had a look at WorkflowMultiBranchProject and this is the way to go, but I need to find some spare time to investigate this and implement it. Also need to have a look how easy it is to rebase the development branch with the current master as this branch contains the environment variables which was also requested.

pascalw commented 6 years ago

@sanderv32 IMO this PR can best be abandoned. The solution implemented here is simply not sufficient and the cleanups are quite small so could easily be reproduced on the master branch.

I'm not currently using Gogs with Jenkins anymore so I personally have little interest being involved in this. I do hope my findings described above were useful to you.