-
Notifications
You must be signed in to change notification settings - Fork 95
Modified build.sh to also run testasroot target #801
Modified build.sh to also run testasroot target #801
Conversation
misc/scripts/build.sh is not running make on testasroot target. Main job of build.shell is to run containers and then do 'make Target' within those containers. Due to some earlier change in build.sh, we had missed to add condition to also run 'testsasroot' target while running our tests locally. This issue was not discovered on drone because while running tests on drone, we do not call build.sh |
What was the testing done? |
@@ -122,7 +122,8 @@ then | |||
elif [ "$1" == "pylint" ] | |||
then | |||
$DOCKER run --rm -it -v $PWD/..:$dir -w $dir $pylint_container $MAKE_ESX pylint | |||
elif [ "$1" == "build" ] || [ "$1" == "clean-as-root" ] | |||
elif [ "$1" == "build" ] || [ "$1" == "clean-as-root" ] || [ "$1" == "testasroot" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this the default condition, unless a special rule exists run make using the $plug_container and the $MAKE cmd. Change this to else
instead of elseif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after addressing
- Testing done
- change elsif to else
Addressed the review comment of just having an else condition which will accept all targets. Ran the test locally with ESX and two VMs and tests were working fine. ========================== I could also see in the log that testasroot target is being executed - ../misc/scripts/build.sh testasroot => Running target testasroot Wed Dec 7 18:22:29 UTC 2016 GO15VENDOREXPERIMENT=1 go test github.com/vmware/docker-volume-vsphere/vmdk_plugin/vmdkops |
LGTM post CI run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing it !
Fixes #800