[
https://issues.apache.org/jira/browse/HADOOP-11746?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14496745#comment-14496745
]
Chris Nauroth commented on HADOOP-11746:
----------------------------------------
Thank you for the patch, Allen. The new functionality looks great! Here are a
few comments.
checkstyle.sh
# {{checkstyle_postapply}}: Please correct the indentation on this line.
{code}
cp -p "${BASEDIR}/target/checkstyle-result.xml"
"${PATCH_DIR}/checkstyle-result-patch.xml"
{code}
# Do you think it's worthwhile to move the Python code into its own .py file?
shellcheck.sh
# {{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?
# There are shellcheck warnings on lines 49 and 70. I see shellcheck.sh caught
them in the last run, so that's cool. :-)
whitespace.sh
# From some quick testing of the grep, it's catching trailing spaces, but not
trailing tabs.
test-patch.sh
# {{precheck_without_patch}} and {{check_site}}: The mvn site command that is
echoed is slightly different from what is actually run. The arguments are in a
different order. I wonder if it would be helpful to have a {{echo_and_exec}}
helper function to call everywhere that we do this kind of thing.
# {{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.
# {{check_unittests}}: The echo of the mvn command on line 1736 does not
include the redirection of output to test_logfile as the actual executed
command does.
# {{output_to_console}}: Today I learned that your secret talent is ASCII
elephant art. :-)
# There are shellcheck warnings on lines 1158, 1573, 1732, 1751, and 1995.
# 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.
Are you planning to clean up smart-apply-patch.sh in scope of this jira, or
will that happen elsewhere?
> 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
>
>
> This code is bad and you should feel bad.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
