Conversation
…ocs) Child B of the Yetus master precommit initiative (follows NUTCH-3196). Fix shellcheck errors in bin/crawl and bin/nutch, add shelldocs annotations, restructure docker/Dockerfile for hadolint, exclude parse-js test sample from jshint, and waive DL3018 apk pinning via .hadolint.yaml.
…tion refactor Replace legacy backticks with $() on the cygpath assignment moved into the for-loop by the SC2144 fix so Yetus patch mode no longer reports a new shellcheck note on PR 956.
sebastian-nagel
left a comment
sebastian-nagel
left a comment
There was a problem hiding this comment.
Thanks, @lewismc. Looks good.
The exit values may need some extra review / thinking.
| local=false | ||
| for f in "$NUTCH_HOME"/*nutch*.job; do | ||
| # NUTCH_JOB | ||
| for f in "${NUTCH_HOME}"/*nutch*.job; do |
There was a problem hiding this comment.
If there are two or more job files in NUTCH_HOME, that's already an issue very likely. But ok for this fix. If we are lucky the last job file is the right one.
There was a problem hiding this comment.
I agree. More than one nutch.job already means a messy NUTCH_HOME. This change only makes the glob shellcheck-safe and takes the first match. Happy to fail closed on multiples in a follow-up if you want that.
| # check that hadoop can be found on the path | ||
| if [ $(which hadoop | wc -l ) -eq 0 ]; then | ||
| echo "Can't find Hadoop executable. Add HADOOP_COMMON_HOME/bin to the path or run in local mode." | ||
| exit -1; |
There was a problem hiding this comment.
This is somewhat sensitive because the script bin/crawl expects that bin/nutch generate ... exits with code 1 when no segment was created. All errors should ideally use other error codes.
Of course, this is not 100% working, because the Generator uses Hadoop's ToolRunner which maps exceptions (don't whether all) to 1.
There was a problem hiding this comment.
Maybe we should change the behavior of Generator and exit with a more unique code, in case no segment was created (nothing to fetch). But would be a breaking change.
There was a problem hiding this comment.
Yes this was some education for me. 1 would mean “no segment” for bin/crawl + Generator... which is misleading/incorrect.
A missing Hadoop is now exit 127 (POSIX utility-not-found) in bin/nutch and bin/crawl, so that path is no longer confused with an empty generate. ToolRunner still mapping some exceptions to 1 is unchanged.
Apache Yetus test-patch report
This message was automatically generated. |
|
Hi @sebastian-nagel thanks for the review. In summary, this PR now keeps generate’s A dedicated empty-generate code can be its own JIRA if we want to break that contract... I didn't bother here. I'll leave this open for a while longer, if you want to revisit. Thanks. |
2 participants
PR for NUTCH-3197
Follow-up to NUTCH-3196 (Child A: Yetus false-positive baselines). This is Child B of the Yetus master precommit initiative: fix real defects the gate correctly reported, without taking on historical style debt (Child C) or shellcheck warnings/notes (Child D).
Fork:
lewismc/nutch→apache/nutch(replaces incorrectly opened #956 from upstream branch).Summary
src/bin/crawlandsrc/bin/nutch(SC2071, SC2144, SC2242, SC2145) with behavior-preserving edits$(...)instead of backticks)@audience/@stabilityshelldocs annotations to five helper functions insrc/bin/crawldocker/Dockerfilefor hadolint (combined RUNs, quoting); waive DL3018 via.hadolint.yamlsrc/plugin/parse-js/sample/from jshint via.yetus/excludes.txt(test fixture, not production JS)Product codespell typos are in a linked follow-up PR from the same fork.
Out of scope (sibling issues)
@author, markdownlint → Child C (NUTCH-3198)NUTCH-3197-codespellTest plan
ant clean runtime testshellcheck -S error src/bin/crawl src/bin/nutch— no errors