XComp commented on code in PR #683: URL: https://github.com/apache/flink-web/pull/683#discussion_r1371343654
########## _include/q/_utils.sh: ########## @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) +CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" + +export TOP_PID=$$ +trap 'exit 1' TERM + +function extract_parameter() { + if [[ "$#" != 1 ]]; then + echo "Fatal error: No parameter or too many parameter passed: $@" >&2 Review Comment: ```suggestion echo "Fatal error: No parameter or too many parameters passed: $@" >&2 ``` nit: just to avoid future typo PRs ########## _include/q/_utils.sh: ########## @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) +CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" + +export TOP_PID=$$ +trap 'exit 1' TERM Review Comment: > I found that exit 1 in the function can not exit the script, so I modify the logic, please check it Great finding :-) I verified it :heavy_check_mark: ########## _include/q/_utils.sh: ########## @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) +CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" + +export TOP_PID=$$ +trap 'exit 1' TERM + +function extract_parameter() { + if [[ "$#" != 1 ]]; then + echo "Fatal error: No parameter or too many parameter passed: $@" >&2 + exit_script + fi + parameter_value=`awk -F'"' '/'$1'[ ]*=[ ]*"/{print $2}' $CONFIG_DIR` + if [ "$parameter_value" = "" ]; then + echo "Fatal error: $1 parameter no found valid value." >&2 + exit_script + fi + echo ${parameter_value} +} + +function exit_script() { Review Comment: ```suggestion function trigger_fatal_error() { echo $1 >&2 ``` nit: we could move the error message into the fatal error function to reduce code redundancy even more. ...just as an idea. You don't have to go for it if you think it's too much. The script is small enough to grasp what it's doing, anyway. ########## _include/q/gradle-quickstart.sh: ########## @@ -41,11 +43,11 @@ function mkPackage() { defaultProjectName="quickstart" defaultOrganization="org.myorg.quickstart" defaultVersion="0.1-SNAPSHOT" -defaultFlinkVersion="${1:-1.17.0}" -# flink-docs-master/docs/dev/datastream/project-configuration/#gradle -# passes the scala version prefixed with a _, e.g.: _2.12 -scalaBinaryVersionFromCmdArg="${2/_/}" -defaultScalaBinaryVersion="${scalaBinaryVersionFromCmdArg:-2.12}" +defaultFlinkVersion="$(extract_parameter 'FlinkStableVersion')" +defaultScalaBinaryVersion="$(extract_parameter 'ScalaShortVersion')" Review Comment: I notice that we're not mimicking the old behavior here (the command line parameter is not parsed). Did you do some research on whether that change is valid? ########## _include/q/quickstart-SNAPSHOT.sh: ########## @@ -18,13 +18,16 @@ # limitations under the License. ################################################################################ +source "$(dirname "$0")"/_utils.sh PACKAGE=quickstart +defaultFlinkVersionSnapShot="$(extract_parameter 'FlinkStableShortVersion')-SNAPSHOT" + mvn org.apache.maven.plugins:maven-archetype-plugin:2.4:generate \ -DarchetypeGroupId=org.apache.flink \ -DarchetypeArtifactId=flink-quickstart-java \ - -DarchetypeVersion=${1:-1.17-SNAPSHOT} \ + -DarchetypeVersion=${1:-defaultFlinkVersionSnapShot} \ Review Comment: ```suggestion -DarchetypeVersion=${1:-$defaultFlinkVersionSnapShot} \ ``` ########## _include/q/quickstart.sh: ########## @@ -18,13 +18,16 @@ # limitations under the License. ################################################################################ +source "$(dirname "$0")"/_utils.sh PACKAGE=quickstart +defaultFlinkVersion="$(extract_parameter 'FlinkStableVersion')" + mvn archetype:generate \ -DarchetypeGroupId=org.apache.flink \ -DarchetypeArtifactId=flink-quickstart-java \ - -DarchetypeVersion=${1:-1.17.0} \ + -DarchetypeVersion=${1:-defaultFlinkVersion} \ Review Comment: ```suggestion -DarchetypeVersion=${1:-$defaultFlinkVersion} \ ``` ########## _include/q/gradle-quickstart.sh: ########## @@ -62,6 +64,12 @@ while [ $TRUE ]; do flinkVersion=${flinkVersion:-$defaultFlinkVersion} read -p "Scala version ($defaultScalaBinaryVersion): " scalaBinaryVersion scalaBinaryVersion=${scalaBinaryVersion:-$defaultScalaBinaryVersion} + read -p "Java version ($defaultJavaVersion): " javaVersion + javaVersion=${javaVersion:-$defaultJavaVersion} + read -p "Slf4j version ($defaultSlf4jVersion): " slf4jVersion + slf4jVersion=${slf4jVersion:-defaultSlf4jVersion} + read -p "Log4j version ($defaultLog4jVersion): " log4jVersion + log4jVersion=${log4jVersion:-defaultLog4jVersion} Review Comment: ```suggestion slf4jVersion=${slf4jVersion:-$defaultSlf4jVersion} read -p "Log4j version ($defaultLog4jVersion): " log4jVersion log4jVersion=${log4jVersion:-$defaultLog4jVersion} ``` ########## _include/q/_utils.sh: ########## @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) +CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" + +export TOP_PID=$$ Review Comment: ```suggestion # fatal error handling export PROCESS_PID=$$ ``` ########## _include/q/quickstart-scala.sh: ########## @@ -18,13 +18,16 @@ # limitations under the License. ################################################################################ +source "$(dirname "$0")"/_utils.sh PACKAGE=quickstart +defaultFlinkVersion="$(extract_parameter 'FlinkStableVersion')" + mvn archetype:generate \ -DarchetypeGroupId=org.apache.flink \ -DarchetypeArtifactId=flink-quickstart-scala \ - -DarchetypeVersion=${1:-1.17.0} \ + -DarchetypeVersion=${1:-defaultFlinkVersion} \ Review Comment: ```suggestion -DarchetypeVersion=${1:-$defaultFlinkVersion} \ ``` ########## _include/q/quickstart-scala-SNAPSHOT.sh: ########## @@ -18,13 +18,16 @@ # limitations under the License. ################################################################################ +source "$(dirname "$0")"/_utils.sh PACKAGE=quickstart +defaultFlinkVersionSnapShot="$(extract_parameter 'FlinkStableShortVersion')-SNAPSHOT" + mvn org.apache.maven.plugins:maven-archetype-plugin:2.4:generate \ -DarchetypeGroupId=org.apache.flink \ -DarchetypeArtifactId=flink-quickstart-scala \ - -DarchetypeVersion=${1:-1.17-SNAPSHOT} \ + -DarchetypeVersion=${1:-defaultFlinkVersionSnapShot} \ Review Comment: ```suggestion -DarchetypeVersion=${1:-$defaultFlinkVersionSnapShot} \ ``` ########## _include/q/_utils.sh: ########## @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +################################################################################ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +SCRIPT_DIR=$(cd $(dirname "${BASH_SOURCE[0]}") && pwd) +CONFIG_DIR="${SCRIPT_DIR}/../../docs/config.toml" + +export TOP_PID=$$ +trap 'exit 1' TERM + +function extract_parameter() { + if [[ "$#" != 1 ]]; then + echo "Fatal error: No parameter or too many parameter passed: $@" >&2 + exit_script + fi + parameter_value=`awk -F'"' '/'$1'[ ]*=[ ]*"/{print $2}' $CONFIG_DIR` Review Comment: ```suggestion parameter_value="$(awk -F'"' '/'$1'[ ]*=[ ]*"/{print $2}' $CONFIG_DIR)" ``` nit: use `$()` and quotes here as well -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
