Build and test managed plugin support in CI fixes #1020 #1061
Conversation
shuklanirdesh82
force-pushed
the
mgdPluginSuppportCI.shuklanirdesh82
branch
3 times, most recently
from
17533bc
to
6a5af75
Compare
|
CI run: https://ci.vmware.run/vmware/docker-volume-vsphere/1759 is showing the test case steps as per planned at #1020 |
|
@msterin just FYI: CI failure is expected and failing with the known issue not related with this PR. The code is ready for the review. |
There was a problem hiding this comment.
Good change.
My main concern is that the actual test script covers lots of things which seem redundant (like the first 2 checks_ or seem to be Docker test (i.e disable/create volume). I think these parts make the code less maintainable and harder to read, without adding much value. I'd suggest cutting the tests (see comments).
A few more comments/nits, but they are are not blockers
misc/scripts/deploy-tools.sh
Outdated
| do | ||
| TARGET=root@$ip | ||
| log "Cleaning up older files from $TARGET if exists..." | ||
| $SSH $TARGET rm -fr $TMP_LOC |
There was a problem hiding this comment.
not a big deal, but it's better to run 1 ssh , not 3.
Something like `ssh $target "rm -fr $TMP_LOC; $MKDIR_P $TMP_LOC $BUILD_LOC; "
There was a problem hiding this comment.
Sure, I prefer to do that too. I have seen other functions are doing separately hence this proposal.
Thanks for the comment!
misc/scripts/deploy-tools.sh
Outdated
| if [ $? -ne 0 ]; then | ||
| case $FILE_EXT in | ||
| deb) | ||
| log "installing make on ubuntu machine" |
There was a problem hiding this comment.
I think it's easier to install make (if needed) upfront. Something like 'if [ -z "$(which make)" ] l; then ; fi'
instead of trying, installing and trying again
misc/scripts/plugin_sanity_test.sh
Outdated
|
|
||
| # make sure sanity check is on clean slate and row count should be 2 | ||
| row_count=`docker plugin ls | wc -l` | ||
| if [ $row_count != 2 ]; then |
There was a problem hiding this comment.
it'snot clean slate if it's 2 :-)
I am not sure why do we need to check it. the next checl seems to be covering it also
There was a problem hiding this comment.
yeah! I started with $row_count != 2 and later added -z $pluginName .. I will remove the redundant one (this one).
misc/scripts/plugin_sanity_test.sh
Outdated
| echo "plugin_sanity_test: [INFO] Installed plugin name is:" $pluginName | ||
|
|
||
| # make sure plugin name is not empty | ||
| if [ -z $pluginName ]; then |
There was a problem hiding this comment.
you may wrap to take $pluginMName in ""
misc/scripts/plugin_sanity_test.sh
Outdated
|
|
||
| # check plugin is disabled or not .. by default it should be disabled | ||
| echo "plugin_sanity_test: [INFO] check plugin is disabled or not .. by default it should be disabled..." | ||
| enabled=`docker plugin inspect $pluginName -f {{.Enabled}}` |
There was a problem hiding this comment.
This one is good. Why do we need the first 2 checks though ? They don't seem to add any value. just take space/attention
misc/scripts/plugin_sanity_test.sh
Outdated
| # create volume when plugin is disabled state and expects an error | ||
| echo "plugin_sanity_test: [INFO] create volume when plugin is disabled state and expects an error..." | ||
| docker volume create -d $pluginName --name $volumeName | ||
| if [ $? -eq 0 ]; then |
There was a problem hiding this comment.
this seems to be testing Docker internals. I do not think we need that.
There was a problem hiding this comment.
Removed from sanity check.
misc/scripts/plugin_sanity_test.sh
Outdated
|
|
||
| # Enable plugin and verifies it is enabled or not | ||
| echo "plugin_sanity_test: [INFO] Enable plugin and verifies it is enabled or not..." | ||
| docker plugin enable $pluginName |
There was a problem hiding this comment.
same here. I understand the test. but it seems to be more for docker functionality. I think we need to minimize cross-testing docker when it is not expected to impact our code
There was a problem hiding this comment.
removed redundant part.
misc/scripts/plugin_sanity_test.sh
Outdated
| # try to create volume again and expects volume creation this time as plugin is enabled | ||
| echo "plugin_sanity_test: [INFO] try to create volume again and expects volume creation this time as plugin is enabled..." | ||
| docker volume create -d $pluginName --name $volumeName | ||
| if [ $? -ne 0 ]; then |
There was a problem hiding this comment.
Yes ! that's he test :-) !
| # run busybox and write something to this volume | ||
| echo "plugin_sanity_test: [INFO] run busybox and write something to this volume" | ||
| docker run --rm -v $volumeName:/vol1 --name=$containerName busybox touch /vol1/$fileName | ||
| if [ $? -ne 0 ]; then |
There was a problem hiding this comment.
those 3 lines seem to be everywhere.... I'd consider a functoin for that (this is a nit)
There was a problem hiding this comment.
Agree! Those are kept for customized error message. Please let me know what do you think.
There was a problem hiding this comment.
Thanks @msterin !
vmdk_plugin/Makefile
Outdated
| @@ -56,6 +57,7 @@ AFTER_REMOVE := $(SCRIPTS)/install/after-remove.sh | |||
| # | |||
| CHECK := $(SCRIPTS)/check.sh | |||
| BUILD := $(SCRIPTS)/build.sh | |||
| BUILD_TOOLS := $(SCRIPTS)/deploy-tools.sh | |||
There was a problem hiding this comment.
the script is calleed deploy_tools. Why the Makefile vars is called build_tools ? it's confusing
There was a problem hiding this comment.
my bad! thanks for catching.
shuklanirdesh82
force-pushed
the
mgdPluginSuppportCI.shuklanirdesh82
branch
from
001943d
to
c526904
Compare
Build and test managed plugin support in CI
fixes #1020
All steps are automated mentioned at Build and test managed plugin support in CI #1020 (comment)
script check for
makeis installed/present on the docker host if not it installs automaticallyTesting: Yes, locally and CI too.
CI output:
makeis there, install otherwise/CC @msterin