Good investigating, Casey! Yeah, looks like jira.sh just greps a curl of the JIRA page without regard for whether a comment is supposed to refer to the location of a patch to be picked up by Yetus or not. Really seems like there should be a way to have it start by looking at attachments and only if that fails should it start traversing through comments for links.
On Wed, Aug 17, 2016 at 2:33 PM, Casey Brotherton <[email protected]> wrote: > Hello, > > Dima link had a string patch in it's name: > > ( The removed link from the YETUS-309's comment ) > https://github.com/apache/yetus/blob/master/precommit/ > test-patch.d/pylint.sh#L154 > ] > > The regex from jira.sh looks for a start of GITHUB_BASE_URL with no spaces, > ending with patch, but not checking after patch to see that > the link terminates. > && $(${GREP} -c "${GITHUB_BASE_URL}"'[^ ]*patch' > "${PATCH_DIR}/jira") != 0 ]]; then > > github_jira_bridge in github.sh has the same regex in AWK instead of shell, > however, then the string is passed into github_locate_patch, > which indicates that *diff and *pull/[0-9]+ are both legal. > > There is also a chance for github_locate_patch to call github_breakup_url, > even though it was called before github_locate_patch > > The logging from github.sh indicates that the return code is "1" when the > URL doesn't fit. > Given that, it should be easy to change the if-else in jira.sh to an > if-check-continue to another-if > > Sean, or Allen, if you open two jiras, I would be glad to work on a > proposed patch and could have it posted to the Jiras by Monday. > > ( I would want to dig deeper on what rules there are for the github patch > links, and not assume that they should all end with white space ) > > Thanks, > Casey > > On Wed, Aug 17, 2016 at 3:27 PM, Dima Spivak <[email protected]> wrote: > > > FWIW, I edited my original JIRA comment to not link to a GH line and the > > pre-commit process kicked off properly, so looks like your hunch (Busbey) > > and your explanation (Allen) were both right. > > > > > > > > On Aug 17, 2016, at 12:18 PM, Sean Busbey <[email protected]> wrote: > > > > > > > > The patch is clearly from format-patch, but the yetus output shows: > > > > > > That doesn't matter because ... > > > > > > > > > > > YETUS-309 appears to be a Github PR. Switching Modes. > > > > > > ... as soon as Yetus picks up something that it thinks is a > > github > > > PR, it switches to github mode and there is no mechanism for it to go > > back. > > > > > > > > > > [Wed Aug 17 19:14:10 UTC 2016 DEBUG]: github: test-patch is not a > pull > > > request # > > > > [Wed Aug 17 19:14:10 UTC 2016 DEBUG]: generic_locate_patch: failed to > > > > download the patch. > > > > ERROR: Unsure how to process YETUS-309. > > > > > > > > > > > > The only github related thing I see on the JIRA is a link in a > comment > > > > from Dima pointing to one of our source lines. My guess is that we're > > > > claiming github-pr based on just that link. > > > > > > > > > > > > plausible? > > > > > > Completely. But it's really two separate bugs: > > > > > > * the JIRA plugin's github bridge that tries to detect if it > is a > > > JIRA w/a link to github probably needs to get it's regex strengthened a > > bit. > > > * if the github plugin detects it isn't really a github PR it > > > should really have a way to send it back to JIRA if the bridge > collapses. > > > > > > Relatedly, it's been requested that the bridge should detect > > which > > > one is "newer" and use that, but that's *really* hard to do efficiently > > > without making a ton of queries all over the place. > > > > > > > > > > > > > > > -- > > -Dima > > > > > > -- > Casey J. Brotherton > Customer Operations Engineer > [image: www.cloudera.com] <http://www.cloudera.com> > -- -Dima
