Remove SKIP_TESTS env var option #138

nitisht · 2017-09-14T11:31:28Z

User can pass comma separated names of SDKs to be run in Mint tests.

Fixes: #134

balamurugana · 2017-09-14T14:32:24Z

README.md

@@ -23,8 +23,7 @@ Set environment variables to pass test target server details to the docker conta
 - `ACCESS_KEY`     - Access Key of the server. Defaults to Minio Play Access Key.
 - `SECRET_KEY`     - Secret Key of the server. Defaults to Minio Play Secret Key.
 - `ENABLE_HTTPS`   - Set to 1 to send HTTPS requests on SSL enabled deployment. Defaults to 0.
- `MINT_DATA_DIR`       - Data directory for SDK tests. Defaults to data directory created by `build/data/install.sh` script.
- `SKIP_TESTS`     - `','` separated list of SDKs to ignore running. Empty by default. For example, to skip `minio-js` and `awscli` tests, use `export SKIP_TESTS=minio-js,awscli`.
+- `MINT_DATA_DIR`  - Data directory for SDK tests. Defaults to data directory created by `build/data/install.sh` script.


there is no build/data/install.sh. I don't think you need to describe how it is created

I just removed reference to SKIP_TESTS here. Will send a refactor/cleanup for README in a separate PR as a fix for #135

OK. then you won't need to change this line in this PR

balamurugana · 2017-09-14T14:40:39Z

mint.sh

@@ -76,16 +64,23 @@ function main()
    ## $MINT_MODE is used inside every sdks.
    echo "To get intermittent logs, 'sudo docker cp ${CONTAINER_ID}:/mint/log /tmp/mint-logs'"

-    for sdk_dir in "$TESTS_DIR"/*; do
+    if [ $# -gt 0 ]; then 
+        IFS=',' read -ra run_list <<<"${@}"


this needs to be in global

RUN_LIST=( "$@" ) is good enough for an array.

Write the logic below

if [ "${#RUN_LIST[@]}" -ne 0 ]; then for sdk in "${RUN_LIST[@]}"; do ## run the test done else for sdk in "$TEST_DIR"/*; do ## run the test done fi

balamurugana · 2017-09-14T14:41:12Z

mint.sh

@@ -114,3 +109,5 @@ trap 'echo -e "\nAborting Mint..."; kill $main_pid' SIGINT SIGTERM

 # wait for main to complete
 wait
+
+exec


Why do you need exec here?

This is needed by Docker to properly pass arguments sent to docker run <image> <args>.

Quote from docs:

Command line arguments to docker run <image> will be appended after all elements in an exec form ENTRYPOINT, and will override all elements specified using CMD. This allows arguments to be passed to the entry point, i.e., docker run <image> -d will pass the -d argument to the entry point. You can override the ENTRYPOINT instruction using the docker run --entrypoint flag.

Ref: https://docs.docker.com/engine/reference/builder/#entrypoint

Perhaps a comment regarding this would be better.. because naked exec might get removed in future without knowing this context.

harshavardhana · 2017-09-14T18:40:03Z

mint.sh

@@ -114,3 +109,5 @@ trap 'echo -e "\nAborting Mint..."; kill $main_pid' SIGINT SIGTERM

 # wait for main to complete
 wait
+
+exec


Perhaps a comment regarding this would be better.. because naked exec might get removed in future without knowing this context.

nitisht · 2017-09-15T06:03:13Z

updated the PR @harshavardhana @balamurugana please check

balamurugana · 2017-09-15T14:38:41Z

docker-entrypoint.sh

+#  limitations under the License.
+#
+
+run() {


run function is a overkill. you could simply have

./mint.sh "$@" & mint_pid=$! trap 'echo -e "\nAborting Mint on signal..."; kill $main_pid' SIGINT SIGTERM wait

balamurugana · 2017-09-15T14:45:25Z

mint.sh

+        echo "done in $duration"
+    else
+        echo "FAILED in $duration"
+    fi


you would need to return $rv

balamurugana · 2017-09-15T14:46:44Z

mint.sh

-main "$@" &
-main_pid=$!
-trap 'echo -e "\nAborting Mint..."; kill $main_pid' SIGINT SIGTERM
+main


you would still need to pass "$@" for future use

balamurugana · 2017-09-15T14:47:26Z

mint.sh

-main "$@" &
-main_pid=$!
-trap 'echo -e "\nAborting Mint..."; kill $main_pid' SIGINT SIGTERM
+main

 # wait for main to complete
 wait


this is no more applicable

balamurugana · 2017-09-15T14:48:58Z

Dockerfile.dev

@@ -56,4 +56,5 @@ COPY postinstall.sh /mint
 RUN /mint/postinstall.sh

 COPY mint.sh /mint/mint.sh
-CMD ["/mint/mint.sh"]
+COPY docker-entrypoint.sh /mint/docker-entrypoint.sh
+ENTRYPOINT ["/mint/docker-entrypoint.sh"]


just entrypoint.sh is more clear

nitisht · 2017-09-18T08:06:43Z

Updated @balamurugana

balamurugana · 2017-09-18T15:06:50Z

entrypoint.sh

+
+./mint.sh "$@"  &
+main_pid="$!"
+trap 'echo -e "\nAborting Mint..."; kill -SIGTERM $main_pid' SIGINT SIGTERM


Why do you want send sigterm than default?

balamurugana · 2017-09-18T15:07:31Z

entrypoint.sh

+
+# A signal emitted while waiting will make the wait command return code > 128
+# Wrap it in a loop that doesn't end before the process is indeed stopped
+while kill -0 $main_pid > /dev/null 2>&1; do


unable to understand why you need this while loop

nitisht · 2017-09-18T17:25:17Z

There was some mixup while moving file contents, should be fine now @balamurugana

balamurugana · 2017-09-18T17:28:16Z

entrypoint.sh

+# Get the pid to be used for kill command if required
+main_pid="$!"
+trap 'echo -e "\nAborting Mint..."; kill $main_pid' SIGINT SIGTERM
+wait


Add newline i.e. wait doesn't end with newline

User can instead pass comma separated names of SDKs to be run in Mint tests Fixes minio#134

harshavardhana

LGTM

nitisht requested review from harshavardhana and balamurugana September 14, 2017 11:31

balamurugana reviewed Sep 14, 2017

View reviewed changes

balamurugana requested changes Sep 14, 2017

View reviewed changes

harshavardhana requested changes Sep 14, 2017

View reviewed changes

nitisht force-pushed the remove-skip-tests branch from 4ed2064 to 5b77315 Compare September 15, 2017 04:41

balamurugana requested changes Sep 15, 2017

View reviewed changes

nitisht force-pushed the remove-skip-tests branch from 5b77315 to 86121df Compare September 18, 2017 08:05

balamurugana requested changes Sep 18, 2017

View reviewed changes

nitisht force-pushed the remove-skip-tests branch from 86121df to 595aa6e Compare September 18, 2017 17:24

balamurugana reviewed Sep 18, 2017

View reviewed changes

Remove SKIP_TESTS env var option

785ccc2

User can instead pass comma separated names of SDKs to be run in Mint tests Fixes minio#134

nitisht force-pushed the remove-skip-tests branch from 595aa6e to 785ccc2 Compare September 18, 2017 17:33

balamurugana approved these changes Sep 18, 2017

View reviewed changes

harshavardhana approved these changes Sep 18, 2017

View reviewed changes

nitisht merged commit 703f237 into minio:master Sep 18, 2017

nitisht deleted the remove-skip-tests branch September 18, 2017 20:19

halfcrazy mentioned this pull request Apr 8, 2018

add listobject test #268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SKIP_TESTS env var option #138

Remove SKIP_TESTS env var option #138

nitisht commented Sep 14, 2017

balamurugana Sep 14, 2017

nitisht Sep 14, 2017 •

edited

Loading

balamurugana Sep 14, 2017

balamurugana Sep 14, 2017

balamurugana Sep 14, 2017

nitisht Sep 14, 2017

harshavardhana Sep 14, 2017

harshavardhana Sep 14, 2017

nitisht commented Sep 15, 2017

balamurugana Sep 15, 2017

balamurugana Sep 15, 2017

balamurugana Sep 15, 2017

balamurugana Sep 15, 2017

balamurugana Sep 15, 2017

nitisht commented Sep 18, 2017

balamurugana Sep 18, 2017

balamurugana Sep 18, 2017

nitisht commented Sep 18, 2017

balamurugana Sep 18, 2017 •

edited

Loading

nitisht Sep 18, 2017

harshavardhana left a comment

Remove SKIP_TESTS env var option #138

Remove SKIP_TESTS env var option #138

Conversation

nitisht commented Sep 14, 2017

Choose a reason for hiding this comment

nitisht Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitisht commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitisht commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitisht commented Sep 18, 2017

balamurugana Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana left a comment

Choose a reason for hiding this comment

nitisht Sep 14, 2017 •

edited

Loading

balamurugana Sep 18, 2017 •

edited

Loading