[ https://issues.apache.org/jira/browse/HADOOP-11746?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Allen Wittenauer updated HADOOP-11746: -------------------------------------- Attachment: HADOOP-11746-16.patch -16: * Fixed, etc through all of the [~cnauroth]'s review comments. Much thanks! Some notes: {{checkstyle.sh}} bq. 2. Do you think it's worthwhile to move the Python code into its own .py file? I've been debating this myself. Two advantages of doing it this way: * the whole plug-in is self contained. * the dev-support patch detection is a lot easier {{shellcheck.sh}} bq. 1. {{shellcheck_private_findbash}}: This looks for files in bin and sbin. Do we also need libexec, which is currently used in hadoop-kms and hadoop-hdfs-httpfs? Great catch. I also added shellprofile.d while I was there! {{whitespace.sh}} Ugh. Good point. That should be [[:blank:]] not [[:space:]]. Easy fix. This also means this works in non-C locales a bit better. {{testpatch.sh}} bq. 2. {{determine_needed_tests}}: I believe this would not identify tests for a patch that contained only changes in test resources (not Java test code). Examples of this include testConf.xml and editsStored and editsStored.xml. Relevant code: {code} elif [[ ${i} =~ \.c$ ... || ${i} =~ src/test ... ]]; then hadoop_debug "tests/native: ${i}" add_test javac add_test unit {code} It should. If *any* file has src/test in its path, it will trigger the javac and unit tests. It's really not "tests/native", so maybe that should get renamed to something else I guess. (This is counter to tests/java which also triggers javadoc tests) bq. 4. {{output_to_console}}: Today I learned that your secret talent is ASCII elephant art. I don't know what you are talking about. Maybe your patch download was bad? ;) bq. 5. There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995. Fixed or disabled all of these except two, which are a) false positives and b) extremely hard to escape because they are actually awk commands in a multiline pipe. bq. 6. The old test-patch.sh output always included a hyperlink to the patch file that it ran. Can we please add that? I always found that helpful for knowing exactly which patch it was reporting on, in case multiple revisions got uploaded quickly. It actually does this when the patch provided is from either a JIRA or a URL. This is consistent with the old code. FWIW, I've uploaded the same patch into HADOOP-11820 and ran it in jenkins mode. You'll see the shellcheck #'s and the patch URL there as well. Check out that execution time. :D :D > rewrite test-patch.sh > --------------------- > > Key: HADOOP-11746 > URL: https://issues.apache.org/jira/browse/HADOOP-11746 > Project: Hadoop Common > Issue Type: Test > Components: build, test > Affects Versions: 3.0.0 > Reporter: Allen Wittenauer > Assignee: Allen Wittenauer > Attachments: HADOOP-11746-00.patch, HADOOP-11746-01.patch, > HADOOP-11746-02.patch, HADOOP-11746-03.patch, HADOOP-11746-04.patch, > HADOOP-11746-05.patch, HADOOP-11746-06.patch, HADOOP-11746-07.patch, > HADOOP-11746-09.patch, HADOOP-11746-10.patch, HADOOP-11746-11.patch, > HADOOP-11746-12.patch, HADOOP-11746-13.patch, HADOOP-11746-14.patch, > HADOOP-11746-15.patch, HADOOP-11746-16.patch > > > This code is bad and you should feel bad. -- This message was sent by Atlassian JIRA (v6.3.4#6332)