[FIXED JENKINS-28506] - Git fails to trigger from polling for refs/heads/ style branch name #324
Conversation
Branches now look like:
* 74ae8c2 (b_namespace3/feature4) Add file b_namespace3/feature4 on branch b_namespace3/feature4
| * 73d4779 (a_tests/b_namespace3/feature3) Add file a_tests/b_namespace3/feature3 on branch a_tests/b_namespace3/feature3
|/
| * b00d0c5 (tag: TagA, branchForTagA) Local branch 'branchForTagA'
|/
| * dde47cb (tag: TagBAnnotated, branchForTagBAnnotated) Local branch 'branchForTagBAnnotated'
|/
| * 3e73b26 (a_tests/b_namespace2/master) Local branch 'a_tests/b_namespace2/master'
|/
| * d940b84 (a_tests/b_namespace3/master) Local branch 'a_tests/b_namespace3/master'
|/
| * 0b97e49 (b_namespace3/master) Local branch 'b_namespace3/master'
|/
| * 1aeaf86 (a_tests/b_namespace1/master) Local branch 'a_tests/b_namespace1/master'
|/
* 86e6eec (HEAD -> master) Initial commit
Adds new branches to test JENKINS-28506, introduced in changes between
git plugin 2.3.4 and 2.3.5.
Reduce special cases in trigger tests to 1 isChangeExpected() method. Several of the tests show known existing remote polling issues. Those known issues cannot be resolved without significant risk to backward compatibility, so they are intentionally not being fixed.
| // convert head `refs/*` into shortcut notation `remote/branch` | ||
| String name = head; | ||
| if (head.startsWith("refs/heads/")) name = remote + "/" + head.substring(11); | ||
| else if (head.startsWith("refs/tags/")) name = remote + "/" + head.substring(10); |
There was a problem hiding this comment.
Again hardcode, this wouldn't work with ref/notes or any other git based solution. refs/heads is just a DEFAULT for git branches.
I can propose add new UI specifier as "branch specifier" (that is only for branches), but some 'git refspec specifier' that will work right as git documentation and move matching to this classes.
There was a problem hiding this comment.
So you suggest to match refs/*/ ? I never heard about refs/notes usage with a jenkins job, do you know a use-case for this ?
Also, Branch Specifier help tips only claim to support refs/heads/, refs/remotes// and refs/tags/, so anything else to actually work would be only by chance
I'd like such a stricter approach you suggest, need to be carreful with legacy config conversion
There was a problem hiding this comment.
@ndeloof you can even do github PR builds without any PR plugin, github stores all PRs in refs/pull/$ID/{head,merge} (git scm can do polling against it) . This is not documented but this is how all gh plugins works https://github.com/jenkinsci/github-pullrequest-plugin/blob/master/README.md (Configuration part).
Also jenkinsci/github-plugin#39 probably unrelated, but in general refs hardcodes raising many questions.
There was a problem hiding this comment.
Yes I know ghprb plugin relies on refs/pull, but this require some additional refspec configured (admittedly not used here, should be considered). I never said this is feature complete, just want JENKINS-28506 fixed, future improvements are still possible to offer more flexibility
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
@MarkEWaite all test pass now :D but as you know git-plugin test suite do only check the tip of the iceberg, so a finer review is welcome. also @reviewbybees |
oleg-nenashev
changed the title
| @@ -263,7 +263,7 @@ | |||
| <dependency> | |||
| <groupId>org.jenkins-ci.plugins</groupId> | |||
| <artifactId>matrix-project</artifactId> | |||
| <version>1.4.1</version> | |||
| <version>1.4</version> | |||
There was a problem hiding this comment.
Hm? Is it related somehow?
There was a problem hiding this comment.
Never mind, I've finally read the Wiki. This Matrix Project thing should have been released as 2.0 at least
There was a problem hiding this comment.
you just can't run mvn hpi:run with 1.4.1 - make it hard to test intermediate commits :P
There was a problem hiding this comment.
Would it be better to drop this change from this commit and instead merge #322 ? That would remove one more pull request from the backlog and make this change explicit
Is there a matching change which will allow the JUnit plugin to work with 1.580 (see that preceding pull request for Kohsuke's reference to the JUnit special needed)
|
I won't return from vacation for another week. Could you check with the submitter of the original bug if he's willing to test a pre-release of the plugin before I return? Does this need a matching pre-release of the git client plugin (some form of 1.18.0 pre-release)? The ISO date format parsing bug is also resolved in this, but will it interact badly with the changelog format written by git client plugin 1.16.1? |
|
removed 451c47b, rebased for cleanup of fixup commits |
Problem is visible in: https://github.com/MarkEWaite/git-plugin/tree/master-alexpasca-master That includes the git plugin commits through jenkinsci/git-plugin@06415b9 It also includes the fix for JENKINS-28506 as represented in jenkinsci/git-plugin#324 It also includes a minor fix from Alex Pasca: jenkinsci/git-plugin#325
[FIXED JENKINS-28506] - Git fails to trigger from polling for refs/heads/ style branch name
Proposed alternative to #323
the actual fix is to split BranchSpec matching check in two steps:
refs/(heads|tags)/*remote/branch