Details
-
Sub-task
-
Status: Resolved
-
Blocker
-
Resolution: Fixed
-
HADOOP-12111
-
None
-
Incompatible change
Description
WARNING: this is a fairly big project.
See first comment for a brain dump on the issues.
Attachments
Attachments
- HADOOP-12129.HADOOP-12111.00.patch
- 33 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.01.patch
- 41 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.02.patch
- 44 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.03.patch
- 63 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.04.patch
- 71 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.05.patch
- 74 kB
- Allen Wittenauer
- HADOOP-12129.HADOOP-12111.06.patch
- 74 kB
- Allen Wittenauer
Issue Links
- is duplicated by
-
HADOOP-12312 Findbugs HTML report link shows 0 warnings despite errors
- Resolved
- is required by
-
HADOOP-12257 rework build tool support; add gradle; add scala
- Resolved
Activity
-00:
- this is incomplete; many todo's
- move the logic of patch downloading to a plugin
- flesh out more of the github support
- move the console output to a 'fake' bug system plugin
- fix a few bugs here and there
- change more local's to declare's
- now run through all of the bug systems to allow posting comments to multiple systems at once.
- break write_comment stability
- change ISSUE_RE to JIRA_ISSUE_RE
- we might support more than http URLs now, but I need to test it....
- fix a few offline bits that were missing
- jira now uses username/pass much more than it did
- jira should work on non-apache setups now (altho we really need to do more RESTy things rather than depending upon jira-cli.)
- may have found a bug with shellcheck wrt declare -a...
- removed a few dead vars
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7422/console in case of problems.
Hmm. Clearly some bugs, but more troubling is it looks like this might not be compatible with the old reexec system.
OK, I've figured out how to remove the JIRA CLI requirement. This is important because (at least the version with the least encumbered license) uses SOAP, an interface that Atlassian is disabling everywhere.
HOWEVER.
It means using curl because I could not get this work with wget no matter what I tried.. So I'm going to switch all the wgets to curls.
(cc: abayer , who might be interested in this detail.)
-01:
- switch from wget and jira cli to tools to curl + jira REST API
- issue determination bugfix
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7427/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 20s | Patch does not generate ASF License warnings. |
-1 | shellcheck | 0m 11s | The applied patch generated 6 new shellcheck issues (total was 22, now 28). |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 37s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12749398/HADOOP-12129.HADOOP-12111.01.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site shellcheck |
uname | Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build@2/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
shellcheck | https://builds.apache.org/job/PreCommit-HADOOP-Build/7427/artifact/patchprocess/diff-patch-shellcheck.txt |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7427/console |
This message was automatically generated.
-02:
- fix some bugs with curl's usage in smart-apply-patch
- github support works! both basic and token for auth!
- jira plugin now has some simple/basic support of switching to github when it detects a pull request in the comments
- removed jira-cmd parse arg since we don't need it anymore
- some hard-coded sed's moved to use the sed var
- jira was forcing it's header into github comments
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7434/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 31s | Patch does not generate ASF License warnings. |
-1 | shellcheck | 0m 12s | The applied patch generated 2 new shellcheck issues (total was 22, now 24). |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 48s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12749721/HADOOP-12129.HADOOP-12111.02.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site unit shellcheck |
uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
shellcheck | https://builds.apache.org/job/PreCommit-HADOOP-Build/7434/artifact/patchprocess/diff-patch-shellcheck.txt |
JDK v1.7.0_55 Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7434/testReport/ |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7434/console |
This message was automatically generated.
-03:
- many, many, many bug fixes
- moar documentation!
- jira<->github bridges work much better now and truly are bi
- branch detection now done on a per-bug system basis
- basic branch detection on github PRs
- plugins can now write comments to bug systems that have review capabilities. shellcheck and github are only ones that currently support this
- github pull request links on the command line is much smarter
- better authentication done
- a few more globals killed to clean things up more
On the testing front, after -02, I've been using this patch to develop against github directly (using an oauth token to write back to my PR!) without any interaction with JIRA at all, except for an occasional "I didn't break anything right?". I'm much more confident now that the interfaces make sense and work. Still need to do more documentation here, esp since I know we need to write a plugin for reviewboard at some point.
That said, this is a BIG patch (as expected) and it'd be good for folks to start digging into it. Most of the major work is done and everything should be relatively small bug fixes from here on out!
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7458/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 22s | Patch does not generate ASF License warnings. |
+1 | shellcheck | 0m 13s | There were no new shellcheck issues. |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 41s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12750217/HADOOP-12129.HADOOP-12111.03.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site unit shellcheck |
uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
JDK v1.7.0_55 Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7458/testReport/ |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7458/console |
This message was automatically generated.
-04:
- some doc fixes/additions/cleanup
- fix some leaky variables
- point personalities to their github equivalents and set more reasonable defaults
- added some code comments
- fixed a minor bug with GH pull requests given on the command line
- force GH to use v3 API
- reformat the REST conversion bits to be a bit cleaner
- whitespace now writes line comments
- fix a bug where if the JIRA issue that was given as input was also a branch name, we don't switch to that branch
- if we can't write to a system because of creds, report it.
- guess_patch_file would throw errors if the input file didn't exist. the new locate_patch code can trigger this condition whereas before it didn't.
- bugysstem line comments now take a header so that plugins can report which plugin actually generated the message
- renamed bugsystem_output to bugsystem_finalreport to better reflect reality
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7462/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 15s | Patch does not generate ASF License warnings. |
-1 | shellcheck | 0m 11s | The applied patch generated 1 new shellcheck issues (total was 22, now 23). |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 32s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12750358/HADOOP-12129.HADOOP-12111.04.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site unit shellcheck |
uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
shellcheck | https://builds.apache.org/job/PreCommit-HADOOP-Build/7462/artifact/patchprocess/diff-patch-shellcheck.txt |
JDK v1.7.0_55 Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7462/testReport/ |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7462/console |
This message was automatically generated.
So I've found an interesting bug, but frankly, it's too hard to fix and a lot of it goes back to the brokenness of the github API.
If a file has been modified in two different commits to the point that they show up as different entries even in the github UI, then our comment on the line is going to be probably on the wrong line, if the github API even accepts it. The only way to fix this is to figure out which commit id hash applies to which line change.
A good workaround is really to recommend people to rebase the changes to be in as few commits as possible.
+ +* pluginname_writecomment + + - Given text input, write this output to the bug system as a comment. NOTE: It is the bug system's responsibility to format appropriately. + +* pluginname\_linecomments + + - This function allows for the system to write specific comments on specific lines if the bug system supports code review comments. + +* pluginname_finalreport +
the first and second bullet points should use \ to escape the _
-* ISSUE\_RE is to help test-patch when talking to JIRA. It helps determine if the given project is appropriate for the given JIRA issue. +* JIRA\_ISSUE\_RE is to help test-patch when talking to JIRA. It helps determine if the given project is appropriate for the given JIRA issue. There are similar variables for GITHUB.
Give a pointer to the variable or say what it is? It's GITHUB_ISSUE, and without other context this paragraph would have made me presume GITHUB_ISSUE_RE.
-* wget +* curl
yay!
+ +A new practice within the ASF is to use a service such as GitHub and its Pull Request (PR) feature. test-patch supports many forms of providing PR support. +
nit: I don't think we need to mention the ASF use here, since our goal is larger.
+ ${CURL} --silent \ + --output "${PATCH_DIR}/jira" \ + --location \ + "http://issues.apache.org/jira/browse/${PATCH_OR_ISSUE}"
nit: maybe different issue. can we just start at the https url? also maybe we need some link about setting up https support for curl. and maybe a warning that you should trust your issue tracker since we use --location (though this is very minor, given our broader context).
+# personalities can override the following settings: + +# Web interface URL. +GITHUB_BASE_URL="https://github.com" + +# API interface URL. +GITHUB_API_URL="https://api.github.com" +
does this mean we can support github enterprise installations?!
(looks like barring the note about jira issues that point at github. sweet)
-## @returns ${JIRACLI} exit code +## @returns ${CURL} exit code
so long as we're changing, replace with "exit code from posting to jira" so that we don't leak how we talk to jira?
+ for bug in ${BUGSYSTEMS}; do + if declare -f ${bug}_write_comment >/dev/null; then + "${bug}_write_comment" "${commentfile}" + fi + done
How will this play out for ASF projects that have github integration in place? I think we'll write to github and jira, and then github will write to jira. can we add a flag that picks one and defaults to "all"?
+ while read -r line; do + { + printf "%s" $(echo "${line}" | cut -f1-2 -d:) + echo "${comment}" + } >> "${tmpfile}"
As of shellcheck 3.3.7 that precommit flag still shows up:
In dev-support/test-patch.d/whitespace.sh line 29: printf "%s" $(echo "${line}" | cut -f1-2 -d:) ^-- SC2046: Quote this to prevent word splitting.
Give a pointer to the variable or say what it is? It's GITHUB_ISSUE, and without other context this paragraph would have made me presume GITHUB_ISSUE_RE.
Good catch, especially since GITHUB_ISSUE_RE doesn't exist anymore. (It did when I wrote that.) Specifically called out GITHUB_REPO which is the functional replacement.
maybe different issue. can we just start at the https url?
Yes, we should.
also maybe we need some link about setting up https support for curl. and maybe a warning that you should trust your issue tracker since we use --location (though this is very minor, given our broader context).
Well, that particular code is for smart-apply-patch and specifically hard-coded to the ASF jira. De-Apache-ing smart-apply-patch to point to another JIRA system and add GH support, etc, is a different project for sure. At some point, we need to tie smart-apply-patch into test-patch's plugin code. As long as we keep teasing code out of test-patch, this will get easier and easier to do.
does this mean we can support github enterprise installations?! (looks like barring the note about jira issues that point at github. sweet)
Someone needs to test it, but yes, I think I've gotten the necessary plumbing in place to support it.
can we add a flag that picks one and defaults to "all"?
Add --bugcomments. It takes a list but one always gets the console.
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7483/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 19s | Patch does not generate ASF License warnings. |
+1 | shellcheck | 0m 12s | There were no new shellcheck issues. |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 37s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12750663/HADOOP-12129.HADOOP-12111.05.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site unit shellcheck |
uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
JDK v1.7.0_55 Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7483/testReport/ |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7483/console |
This message was automatically generated.
-06:
- fix issue with --bugcomments default
- add --linecomments to turn those on and off as well
A patch to the files used for the QA process has been detected.
Re-executing against the patched versions to perform further tests.
The console is at https://builds.apache.org/job/PreCommit-HADOOP-Build/7487/console in case of problems.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | precommit patch detected. |
+1 | site | 0m 0s | |
0 | @author | 0m 0s | Skipping @author checks as test-patch.sh has been patched. |
-1 | test4tests | 0m 0s | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
+1 | site | 0m 0s | the patch passed |
+1 | asflicense | 0m 15s | Patch does not generate ASF License warnings. |
+1 | shellcheck | 0m 12s | There were no new shellcheck issues. |
+1 | whitespace | 0m 0s | Patch has no whitespace issues. |
0m 33s |
Subsystem | Report/Notes |
---|---|
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12750717/HADOOP-12129.HADOOP-12111.06.patch |
JIRA Issue | |
git revision | |
Optional Tests | asflicense site unit shellcheck |
uname | Linux asf906.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /home/jenkins/jenkins-slave/workspace/PreCommit-HADOOP-Build/patchprocess/dev-support-test/personality/hadoop.sh |
Default Java | 1.7.0_55 |
Multi-JDK versions | /home/jenkins/tools/java/jdk1.8.0:1.8.0 /home/jenkins/tools/java/jdk1.7.0_55:1.7.0_55 |
shellcheck | v0.3.3 (This is an old version that has serious bugs. Consider upgrading.) |
JDK v1.7.0_55 Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/7487/testReport/ |
Max memory used | 49MB |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/7487/console |
This message was automatically generated.
Thanks for the review!
Committed.
I'll write a release note later today.
There are a bunch of interleaved issues in play here. This is a summary of some of the discussions that sekikn and I had around these topics.
At a minimum, I think we need to do this list sooner rather than later:
a) pull more of the JIRA-specific code out of locate_patch and into the jira.sh plugin
b) support the not-patch-available-but-referenced-in-JIRA-comment use case