[
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)
