You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/10/06 01:23:06 UTC
[GitHub] [openwhisk-release] style95 opened a new pull request #349: Add a flag to remove the temporal directory.
style95 opened a new pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] style95 commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500054944
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
Updated according to your suggestions.
But I keep it enabled by default.
Should I change it to be disabled by default?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500037641
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
```suggestion
REMOVE_DIR=${5:-false}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500037641
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
```suggestion
# the last argument is optional and if set to 'cleanup', the script deletes the scratch space at completion
REMOVE_DIR=${5:-false}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] style95 commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500014091
##########
File path: tools/rcverify.sh
##########
@@ -216,3 +218,9 @@ echo $(tput setaf 6)
echo run the following command to remove the scratch space:
echo " rm -rf '$DIR'"
echo $(tput sgr0)
+
+if [ "$REMOVE_DIR" = true ]; then
Review comment:
Updated accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r499987120
##########
File path: tools/rcverify.sh
##########
@@ -216,3 +218,9 @@ echo $(tput setaf 6)
echo run the following command to remove the scratch space:
echo " rm -rf '$DIR'"
echo $(tput sgr0)
+
+if [ "$REMOVE_DIR" = true ]; then
Review comment:
if removing directory maybe don't emit the message above? That is turn this into an if then else?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500037966
##########
File path: tools/rcverify.sh
##########
@@ -212,7 +214,15 @@ packageJsonCheckVersion "$DIR/$BASE/package.json" $V
printf "scanning package-lock.json for version match..."
packageJsonCheckVersion "$DIR/$BASE/package-lock.json" $V
-echo $(tput setaf 6)
-echo run the following command to remove the scratch space:
-echo " rm -rf '$DIR'"
-echo $(tput sgr0)
+if [ "$REMOVE_DIR" = true ]; then
Review comment:
```suggestion
if [ "$REMOVE_DIR" = "cleanup" ]; then
```
only because as a positional argument `true` is ambiguous.
feel free to ignore this suggestion (keep the comparison to `true`).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500038477
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
Hmm actually this sets the default to `true` if the 5th parameter is not given?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#issuecomment-704244783
> One thing to note is, this feature will be enabled by default.
Sorry I missed that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] style95 commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500050275
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
Yes. I mentioned it in the PR description as well.
If we need to keep the previous behavior, would be better to change it to `false`.
But I suppose most people would not need to keep them?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] style95 merged pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
style95 merged pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500246386
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
My reason for keeping the directory around was to manually inspect the release directory.
I don't object to the change, I suppose it's more convenient for most.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500037641
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
```suggestion
REMOVE_DIR=${5:-cleanup}
```
##########
File path: tools/rcverify.sh
##########
@@ -212,7 +214,15 @@ packageJsonCheckVersion "$DIR/$BASE/package.json" $V
printf "scanning package-lock.json for version match..."
packageJsonCheckVersion "$DIR/$BASE/package-lock.json" $V
-echo $(tput setaf 6)
-echo run the following command to remove the scratch space:
-echo " rm -rf '$DIR'"
-echo $(tput sgr0)
+if [ "$REMOVE_DIR" = true ]; then
Review comment:
```suggestion
if [ "$REMOVE_DIR" = "cleanup" ]; then
```
only because as a positional argument `true` is ambiguous.
feel free to ignore the suggestion and merge.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk-release] rabbah commented on a change in pull request #349: Add a flag to remove the temporal directory.
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #349:
URL: https://github.com/apache/openwhisk-release/pull/349#discussion_r500038477
##########
File path: tools/rcverify.sh
##########
@@ -38,6 +38,8 @@ V=${3?"missing version e.g., '3.19.0'"}
# the release candidate, usualy 'rc1'
RC=${4:-rc1}
+REMOVE_DIR=${5:-true}
Review comment:
Hmm actually this sets the default to `true` if the 5th parameter is not given; was your intent to make it the default?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org