You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/06/22 07:42:48 UTC
[GitHub] [iotdb] leonardodalinky opened a new pull request, #6380: Pre-commit-hooks for dev
leonardodalinky opened a new pull request, #6380:
URL: https://github.com/apache/iotdb/pull/6380
Hi my IoTDB friends!👋
🎉After spending days on the git hooks mechanism, the pre-commit hooks for us developers is ready now! 🎉
## Description
The pre-commit hooks enable auto checks before commits. So next time a single `git commit` should do all the style checks automatically.
Currently, the support functionalities are:
* Turn on/off the hook in local config files.
* Auto `mvn spotless:apply` before commits
* Auto `mvn validate` before commits
* Smart selection of only the changed modules for validation
## Installation
All the files are under `tools/git-hooks`.
### Windows
Double click the `install.ps1`, and allow to execute in admin mode. Make sure that the Git for Windows is the default git on Windows.
**Note**: Windows support is based on the [Git for Windows](https://gitforwindows.org/), which provide a minimal shell environment called Git Bash. Actually, all these hooks on Windows are executed under the Git Bash environment, so the hook script can be somewhat platform-independent.
### Unix
```
./install.sh
```
## Configuration
After installation, a `config.sh` shall be created. Modification to the `config.sh` file would take effect the next time you commit.
All the configurable options is in `config.sample.sh`.
```sh
# enable the pre-commit hooks, 0 - off, 1 - on
export IOTDB_GIT_HOOKS=1
# maven path
# If in Git Bash, replace all '\' to '/', and change the driver path from 'C:\' to '/c/
# No escape character is needed.
# Example:
# export IOTDB_MAVEN_PATH="/c/Program Files/apache-maven-3.6/bin/mvn"
export IOTDB_MAVEN_PATH="mvn"
# git path
# If in Git Bash, see 'IOTDB_MAVEN_PATH'
export IOTDB_GIT_PATH="git"
# smart select the changed java module, 0 - off, 1 - on
export IOTDB_SMART_JAVA_MODULES=1
# auto spotless:apply, 0 - off, 1 - on
export IOTDB_SPOTLESS_APPLY=0
# maven validate, 0 - off, 1 - on
export IOTDB_VALIDATE=1
# 0 - discard all, 1 - logs on errors, 2 - stdout, 3 - stdout & logs on errors
export IOTDB_VERBOSE=2
```
**Note 1 for Windows user**: If you are on Windows, pay attention to the `IOTDB_MAVEN_PATH` and `IOTDB_GIT_PATH`. Since the hook script is executed under Git Bash, you must make sure the Git Bash could get to the maven & git binary file correctly. The path expression in Git Bash is slightly different from Windows. Check the examples in config file carefully.
**Note 2 for Windows user**: The easiest way to handle these binary path is to put them into the %PATH% environment variable in Windows. Then the default configuration should work well.
## Uninstall
See `uninstall.ps1` and `uninstall.sh`.
## Pipeline
The overall pipeline of the hook script:
1. `IOTDB_GIT_HOOKS` checks if the hook is on
2. `IOTDB_SMART_JAVA_MODULES` to select only the changed modules
3. `IOTDB_SPOTLESS_APPLY` runs `mvn spotless:apply`
4. `IOTDB_VALIDATE` runs `mvn validate` or `mvn -pl <changed_modules> validate`
The hook is tested on both Linux and Windows.
## Doc
Not sure where to place the entire documentation. Any suggestion?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on a diff in pull request #6380: Pre-commit-hooks for dev
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on code in PR #6380:
URL: https://github.com/apache/iotdb/pull/6380#discussion_r903529628
##########
tools/git-hooks/config.sample.sh:
##########
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+################################################################################
+## Copyright 2022 leonardodalinky(linkyy2000313@gmail.com)
Review Comment:
this license header is not accpetable... All files in ASF project should have the same license header.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] leonardodalinky commented on pull request #6380: Pre-commit-hooks for dev
Posted by GitBox <gi...@apache.org>.
leonardodalinky commented on PR #6380:
URL: https://github.com/apache/iotdb/pull/6380#issuecomment-1166490633
> BTW, in my test case:
>
> * I modify a java file (which violates spotless rules)
> * use `git add ` the file
> * use `git commit -m 'message'`
>
> The `spotless:apply` is triggered, and the java file style is corrected. however, the modification is not committed. (So, we get a git commit log which only commits the java modification we manually made)
>
> Better to find a solution.
I figure out that the indices of files modified in the pre-commit hooks should be updated by end of the script.
And it's solved now by the `update_changed_file_indices` function called after the `spotless:apply`.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on a diff in pull request #6380: Pre-commit-hooks for dev
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on code in PR #6380:
URL: https://github.com/apache/iotdb/pull/6380#discussion_r906750157
##########
docs/zh/Development/ContributeGuide.md:
##########
@@ -120,6 +120,16 @@ plugin](https://github.com/diffplug/spotless/tree/main/plugin-maven) 和 [google
4. 将“将 import 与‘*’搭配使用的类计数”改成 999 或者一个比较大的值。
5. 将“将静态 import 与‘*’搭配使用的名称计数”改成 999 或者一个比较大的值。
+## 使用 git-hooks 自动检查
Review Comment:
Better to claim that this is optional.
##########
tools/git-hooks/README.md:
##########
@@ -0,0 +1,91 @@
+<!--
+
+ 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.
+
+-->
+# Guide to git hooks
+
+The pre-commit hooks enable auto checks before commits. So next time a single `git commit` should do all the style checks automatically.
+
+Currently, the support functionalities are:
+* Turn on/off the hook in local config files.
+* Auto `mvn spotless:apply` before commits
+* Auto `mvn validate` before commits
+* Smart selection of only the changed modules for validation
+
+## Installation
+
+All the files are under `tools/git-hooks`.
+
+### Windows
+
+Double click the `install.ps1`, and allow to execute in admin mode. Make sure that the Git for Windows is the default git on Windows.
+
+**Note**: Windows support is based on the [Git for Windows](https://gitforwindows.org/), which provide a minimal shell environment called Git Bash. Actually, all these hooks on Windows are executed under the Git Bash environment, so the hook script can be somewhat platform-independent.
+
+### Unix
+
+```
+./install.sh
+```
+
+## Configuration
+
+After installation, a `config.sh` shall be created. Modification to the `config.sh` file would take effect the next time you commit.
+
+All the configurable options is in `config.sample.sh`.
+
+```sh
+# enable the pre-commit hooks, 0 - off, 1 - on
+export IOTDB_GIT_HOOKS=1
+# maven path
+# If in Git Bash, replace all '\' to '/', and change the driver path from 'C:\' to '/c/
+# No escape character is needed.
+# Example:
+# export IOTDB_MAVEN_PATH="/c/Program Files/apache-maven-3.6/bin/mvn"
+export IOTDB_MAVEN_PATH="mvn"
+# git path
+# If in Git Bash, see 'IOTDB_MAVEN_PATH'
+export IOTDB_GIT_PATH="git"
+# smart select the changed java module, 0 - off, 1 - on
+export IOTDB_SMART_JAVA_MODULES=1
+# auto spotless:apply, 0 - off, 1 - on
+export IOTDB_SPOTLESS_APPLY=0
Review Comment:
suggest to enable spotless apply by default (as pre-commit is for simplifying the user behavior, so I think if a developer wants to use this tool, he/she may also want to enable the spotless by default)
##########
tools/git-hooks/config.sample.sh:
##########
@@ -0,0 +1,40 @@
+#!/bin/sh
+
+################################################################################
+##
+## 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.
+##
+################################################################################
+
+# enable the pre-commit hooks, 0 - off, 1 - on
+export IOTDB_GIT_HOOKS=1
+# maven path
+# If in Git Bash, replace all '\' to '/', and change the driver path from 'C:\' to '/c/
+# No escape character is needed.
+# Example:
+# export IOTDB_MAVEN_PATH="/c/Program Files/apache-maven-3.6/bin/mvn"
+export IOTDB_MAVEN_PATH="mvn"
+# git path
+# If in Git Bash, see 'IOTDB_MAVEN_PATH'
+export IOTDB_GIT_PATH="git"
+# smart select the changed java module, 0 - off, 1 - on
+export IOTDB_SMART_JAVA_MODULES=1
+# auto spotless:apply, 0 - off, 1 - on
+export IOTDB_SPOTLESS_APPLY=0
Review Comment:
suggest to enable spotless apply by default (as pre-commit is for simplifying the user behavior, so I think if a developer wants to use this tool, he/she may also want to enable the spotless by default)
##########
tools/git-hooks/install.sh:
##########
@@ -0,0 +1,84 @@
+#!/bin/sh
+
+################################################################################
+##
+## 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 directory
+cd $(dirname "$0") || exit 1
+# project directory
+PROJECT_DIR="../.."
+# hooks directory
+HOOKS_DIR="$PROJECT_DIR/.git/hooks"
+# tool directory rel to project
+REL_SCRIPT_DIR="tools/git-hooks"
+
+# config sample filename rel to script dir
+REL_CONFIG_SAMPLE_FILE="config.sample.sh"
+# config filename rel to script dir
+REL_CONFIG_FILE="config.sh"
+
+# color
+Red="\x1B[31m"
+Green="\x1B[32m"
+Yellow="\x1B[33m"
+NC="\x1B[0m"
+
+check_paths() {
+ echo "Checking paths..."
+ if [ ! -d "$PROJECT_DIR" ]; then
+ printf "${Red}ERROR: Project directory '$PROJECT_DIR' does not exist.${NC}\n" >&2
+ exit 1
+ fi
+ if [ ! -d "$HOOKS_DIR" ]; then
+ printf "${Red}ERROR: Hooks directory '$HOOKS_DIR' does not exist.${NC}\n" >&2
+ exit 1
+ fi
+}
+
+install() {
+ # create the relative symlink of pre-commit script
+ if [ -f "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" ] || [ -L "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" ]; then
+ printf "${Yellow}WARN: Overwriting '$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit'${NC}\n"
+ fi
+ ln -sf "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" "$HOOKS_DIR/pre-commit" || exit 1
Review Comment:
Better to echo what we are doing.. e.g., "copy the pre-commit hook to .git/pre-commit..."
##########
docs/Development/ContributeGuide.md:
##########
@@ -153,6 +153,16 @@ In IDEA, you can follow these steps to change those inconsistent style formattin
4. Change 'Class count to use import with '\*'' to 999 or another very large number.
5. Change 'Names to count to use static import with '\*'' to 999 or another very large number.
+## Use git-hooks
Review Comment:
Better to claim that this is optional.
##########
tools/git-hooks/pre-commit:
##########
@@ -0,0 +1,128 @@
+#!/bin/sh
+
+################################################################################
+##
+## 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.
+##
+################################################################################
+
+PROJECT_DIR="../.."
+REL_HOOKS_DIR=".git/hooks"
+
+cd "$(dirname $0)/$PROJECT_DIR" || exit 1
+
+PROMPT="Pre-commit-hooks:"
+
+# default 0 level log path
+LOG_OUT_PATH="/dev/null"
+
+exec_with_log() {
+ if [ "$IOTDB_VERBOSE" -le 1 ]; then
+ eval "$*" > "$LOG_OUT_PATH" 2>&1
+ elif [ "$IOTDB_VERBOSE" -eq 2 ]; then
+ # compatible with git bash
+ eval "$*" 2>&1
+ else
+ eval "$*" 2>&1 | tee "$LOG_OUT_PATH"
+ fi
+}
+
+on_error() {
+ if [ "$#" -eq 0 ]; then
+ echo "ERROR: 'on_error' must have at least 1 args."
+ exit 1
+ fi
+ echo "$PROMPT [ERROR] $*"
+ if [ "$IOTDB_VERBOSE" -eq 1 ] || [ "$IOTDB_VERBOSE" -ge 3 ]; then
+ echo "$PROMPT Log of last step is saved at $LOG_OUT_PATH"
+ fi
+ exit 1
+}
+
+load_config() {
+ . "./$REL_HOOKS_DIR/config.sh"
+}
+
+main() {
+ load_config
+
+ # enable hooks
+ if [ "$IOTDB_GIT_HOOKS" -eq 0 ]; then
+ echo "$PROMPT Skip git hooks."
+ exit 0
+ fi
+
+ # get all changed module path
+ changed_files=$($IOTDB_GIT_PATH diff --name-only --cached)
+ changed_java_modules=""
+ smart_args=""
+ for line in $changed_files; do
+ mod=$(echo "$line" | cut -d '/' -f 1)
+ if [ -f "$mod/pom.xml" ]; then
+ changed_java_modules="${changed_java_modules}${mod},"
+ fi
+ done
+ if [ "$IOTDB_SMART_JAVA_MODULES" -ne 0 ]; then
+ # prepare the smart module selection args
+ echo "$PROMPT Changed java module: $changed_java_modules"
+ if [ -n "$changed_java_modules" ]; then
+ smart_args="-pl $changed_java_modules"
+ fi
+ fi
+
+ # change log level
+ if [ "$IOTDB_VERBOSE" -eq 1 ]; then
+ LOG_OUT_PATH="$(mktemp --suff .log)"
+ elif [ "$IOTDB_VERBOSE" -eq 2 ]; then
+ # not used in git bash
+ LOG_OUT_PATH="/dev/stdout"
+ elif [ "$IOTDB_VERBOSE" -ge 3 ]; then
+ LOG_OUT_PATH="$(mktemp --suff .log)"
+ fi
+
+ # spotless:apply
+ if [ "$IOTDB_SPOTLESS_APPLY" -ne 0 ]; then
+ echo "$PROMPT Apply spotless:apply..."
+ if [ "$IOTDB_SMART_JAVA_MODULES" -eq 0 ] || [ -n "$smart_args" ]; then
+ exec_with_log "\"$IOTDB_MAVEN_PATH\" $smart_args spotless:apply"
+ else
+ echo "$PROMPT No changed module for spotless:apply."
+ fi
+ if [ "$?" -ne 0 ]; then
+ on_error "spotless:apply failed."
+ fi
+ fi
+
+ # maven validate
+ if [ "$IOTDB_VALIDATE" -ne 0 ]; then
+ echo "$PROMPT Validating..."
Review Comment:
echo "$PROMPT Validating... with `mvn validate`"
##########
tools/git-hooks/install.sh:
##########
@@ -0,0 +1,84 @@
+#!/bin/sh
+
+################################################################################
+##
+## 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 directory
+cd $(dirname "$0") || exit 1
+# project directory
+PROJECT_DIR="../.."
+# hooks directory
+HOOKS_DIR="$PROJECT_DIR/.git/hooks"
+# tool directory rel to project
+REL_SCRIPT_DIR="tools/git-hooks"
+
+# config sample filename rel to script dir
+REL_CONFIG_SAMPLE_FILE="config.sample.sh"
+# config filename rel to script dir
+REL_CONFIG_FILE="config.sh"
+
+# color
+Red="\x1B[31m"
+Green="\x1B[32m"
+Yellow="\x1B[33m"
+NC="\x1B[0m"
+
+check_paths() {
+ echo "Checking paths..."
+ if [ ! -d "$PROJECT_DIR" ]; then
+ printf "${Red}ERROR: Project directory '$PROJECT_DIR' does not exist.${NC}\n" >&2
+ exit 1
+ fi
+ if [ ! -d "$HOOKS_DIR" ]; then
+ printf "${Red}ERROR: Hooks directory '$HOOKS_DIR' does not exist.${NC}\n" >&2
+ exit 1
+ fi
+}
+
+install() {
+ # create the relative symlink of pre-commit script
+ if [ -f "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" ] || [ -L "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" ]; then
+ printf "${Yellow}WARN: Overwriting '$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit'${NC}\n"
+ fi
+ ln -sf "$PROJECT_DIR/$REL_SCRIPT_DIR/pre-commit" "$HOOKS_DIR/pre-commit" || exit 1
+ if [ -f "pre-commit" ] && [ ! -x "pre-commit" ]; then
+ printf "${Yellow}ERROR: 'pre-commit' has no execute permission. Try to grant it the 'x' permission.${NC}\n"
+ exit 1
+ fi
+ # create the relative symlink of config file
+ if [ ! -f "$REL_CONFIG_FILE" ]; then
+ # if `config.sh` not exists in this directory, create it from sample
+ test -f "$REL_CONFIG_SAMPLE_FILE" || { echo "ERROR: Could not find '$REL_CONFIG_SAMPLE_FILE'." >&2; exit 1; }
+ cp "$REL_CONFIG_SAMPLE_FILE" "$REL_CONFIG_FILE"
Review Comment:
Better to echo what we are doing..
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] jixuan1989 commented on pull request #6380: Pre-commit-hooks for dev
Posted by GitBox <gi...@apache.org>.
jixuan1989 commented on PR #6380:
URL: https://github.com/apache/iotdb/pull/6380#issuecomment-1166411496
BTW, in my test case:
- I modify a java file (which violates spotless rules)
- use `git add ` the file
- use `git commit -m 'message'`
The `spotless:apply` is triggered, and the java file style is corrected.
however, the modification is not committed.
(So, we get a git commit log which only commits the java modification we manually made)
Better to find a solution.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org