David Caro has posted comments on this change.
Change subject: Adding system tests
......................................................................
Patch Set 9: Code-Review-1
(18 comments)
For the shell scripts a couple rules:
* Quote every variable you use if you are not 100% sure if it should be
unqouted
* Use double square brackets ([[ ]] vs [ ])
* All global variables you are going to use, declared at the top
* When expecting environment variables, add MYVAR="${MYVAR?}" to the globals
list at the top to make sure they are not empty
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/shell-scripts/system_tests.collect_logs.sh
File jobs/confs/shell-scripts/system_tests.collect_logs.sh:
Line 1: #!/bin/bash
Line 2: echo 'shell_scripts/system_tests.collect_logs.sh'
Define all the global variables at the top, also try to ster with the ones that
are required like this:
WORKSPACE="${WORKSPACE?}"
PREFIX="$WORKSPACE/jenkins-deployment-${BUILD_NUMBER?}"
That will make the script fail if any of them is empty when starting up and
will allow easily to see which env vars are used by the script.
Line 3: PREFIX=$WORKSPACE/jenkins-deployment-$BUILD_NUMBER
Line 4:
Line 5: cd $WORKSPACE
Line 6: if [ -d $PREFIX ]
Line 13: rav
no need for -r
Line 18: cp -av $PREFIX/logs/ $WORKSPACE/exported-archives/testenv_logs
Line 19: fi
Line 20:
Line 21: if [ -d $PREFIX/build ]; then
Line 22: find $PREFIX/build -name '*.rpm' -exec rm '{{}}' \;
use rm -f to avoid interactive confirmation in any case
Line 23: cp -av $PREFIX/build $WORKSPACE/exported-archives/build_logs
Line 24: fi
Line 25:
Line 26: rm -rf $PREFIX
Line 22: find $PREFIX/build -name '*.rpm' -exec rm '{{}}' \;
Line 23: cp -av $PREFIX/build $WORKSPACE/exported-archives/build_logs
Line 24: fi
Line 25:
Line 26: rm -rf $PREFIX
really quotes
Line 27:
Line 28: tar cvzf $WORKSPACE/exported-archives.tar.gz exported-archives/
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/shell-scripts/system_tests_common.sh
File jobs/confs/shell-scripts/system_tests_common.sh:
Line 19: ENGINE_DIST=$DISTRO
Line 20: VDSM_DIST=$DISTRO
Line 21: OVIRT_CONTRIB=/usr/share/ovirttestenv/
Line 22:
Line 23: if [ $DISTRO == "el6" ]; then
use double [[ ]]
Line 24: VIRT_CONFIG=$OVIRT_CONTRIB/config/virt/centos6.json
Line 25: DEPLOY_SCRIPTS=$OVIRT_CONTRIB/config/deploy/centos6.scripts.json
Line 26: else
Line 27: echo "Distro not supported"
Line 20: VDSM_DIST=$DISTRO
Line 21: OVIRT_CONTRIB=/usr/share/ovirttestenv/
Line 22:
Line 23: if [ $DISTRO == "el6" ]; then
Line 24: VIRT_CONFIG=$OVIRT_CONTRIB/config/virt/centos6.json
Never mix tabs and spaces, and prefer spaces
Line 25: DEPLOY_SCRIPTS=$OVIRT_CONTRIB/config/deploy/centos6.scripts.json
Line 26: else
Line 27: echo "Distro not supported"
Line 28: exit 1
Line 27: echo "Distro not supported"
Line 28: exit 1
Line 29: fi
Line 30:
Line 31: BRANCH="{version}"
Declare all globals at the start of the script
Line 32: if [ $BRANCH == "ovirt-3.5" ]; then
Line 33:
REPOSYNC_YUM_CONFIG=$OVIRT_CONTRIB/config/repos/ovirt-3.5-external.repo
Line 34: elif [ $BRANCH == "master" ]; then
Line 35:
REPOSYNC_YUM_CONFIG=$OVIRT_CONTRIB/config/repos/ovirt-master-snapshot-external.repo
Line 42: REPOSYNC_DIR=$WORKSPACE/ovirt-repo
Line 43: ENGINE_DIR=$WORKSPACE/ovirt-engine
Line 44: VDSM_DIR=$WORKSPACE/vdsm
Line 45:
Line 46: set -e
this is already set in the shebang
Line 47: chmod g+x $WORKSPACE
Line 48:
Line 49: # Clone templates
Line 50: if [ ! -d $TEMPLATES_DIR ]
Line 46: set -e
Line 47: chmod g+x $WORKSPACE
Line 48:
Line 49: # Clone templates
Line 50: if [ ! -d $TEMPLATES_DIR ]
the '!' is better outside the brackets
use double brackets
quote variable
Line 51: then
Line 52: /usr/share/testenv/sync_templates.py --create $TEMPLATES_CLONE_URL
$TEMPLATES_DIR
Line 53: else
Line 54: /usr/share/testenv/sync_templates.py $TEMPLATES_DIR
Line 48:
Line 49: # Clone templates
Line 50: if [ ! -d $TEMPLATES_DIR ]
Line 51: then
Line 52: /usr/share/testenv/sync_templates.py --create $TEMPLATES_CLONE_URL
$TEMPLATES_DIR
quote vars
Line 53: else
Line 54: /usr/share/testenv/sync_templates.py $TEMPLATES_DIR
Line 55: fi
Line 56:
Line 55: fi
Line 56:
Line 57: # Create $PREFIX for current run
Line 58: testenvcli init \
Line 59: $PREFIX \
avoid tabs
Line 60: $VIRT_CONFIG \
Line 61: --templates-dir=$TEMPLATES_DIR
Line 62: echo '[INIT_OK] Initialized successfully, need cleanup later'
Line 63:
Line 76: testenvcli start
Line 77:
Line 78: # Install RPMs
Line 79: testenvcli ovirt deploy $DEPLOY_SCRIPTS \
Line 80: $OVIRT_CONTRIB/setup_scripts
If you break a command on multiple lines, try to break on each argument block:
mycommand \
--option1 whatever \
--option2_no_arg \
--option3_no_arg \
lonely_arg
instead of
mycommand --option1 \
arg_to_option1 --option2_no_arg \
--option3_no_arg lonely_arg
Line 81:
Line 82: # Start testing
Line 83: testenvcli ovirt runtest $OVIRT_CONTRIB/test_scenarios/bootstrap.py
Line 84: testenvcli ovirt snapshot
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/yaml/jobs/ovirt/system-tests.yaml
File jobs/confs/yaml/jobs/ovirt/system-tests.yaml:
Line 9: - positive-code-review
Line 10: - merged # FIXME
Line 11: version:
Line 12: - master
Line 13: - ovirt-3.5
The version should be just X.Y
Line 14: distro:
Line 15: - el6
Line 16: - el7
Line 17: arch:
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/yaml/scms/ovirt-engine.yaml
File jobs/confs/yaml/scms/ovirt-engine.yaml:
Line 10:
try to use the same indentation levels for all the file (better if you use the
same as other files too)
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/yaml/templates/system_tests.yaml
File jobs/confs/yaml/templates/system_tests.yaml:
Line 1: - job-template:
Line 2: name: '{project}_{version}_system-tests-{distro}-{arch}_{trigger}'
Line 3: node: '{node-filter}'
If you want to be able to run it manually with a custom patch refspec, add here
a parameters section with two string parameters GERRIT_REFSPEC and
GERRIT_BRANCH with default values 'refs/heads/{branch}' and 'origin/{branch}'
respectively
Line 4: triggers:
Line 5: - on-patch-{trigger}:
Line 6: project: '{project}'
Line 7: branch: '{version}'
Line 6: project: '{project}'
Line 7: branch: '{version}'
Line 8: scm:
Line 9: - '{project}-gerrit':
Line 10: branch: '{version}'
Are you sure this should ve just version?
vdms uses ovirt-X.Y and engine ovirt-engine-X.Y.Z branch names
Line 11: - '{other-project}':
Line 12: branch: '{version}'
Line 13: builders:
Line 14: - system-tests:
https://gerrit.ovirt.org/#/c/37069/9/jobs/confs/yaml/triggers/gerrit.yaml
File jobs/confs/yaml/triggers/gerrit.yaml:
Line 22: project-pattern: '{project}'
Line 23: branches:
Line 24: - branch-compare-type: 'PLAIN'
Line 25: branch-pattern: '{branch}'
Line 26: silent: true # FIXME
Don't change this, that will modify ALL the jobs that use this trigger (see the
huge diff in the jenkins job)
Line 27:
Line 28: - trigger:
Line 29: name: on-patch-created-with-files
Line 30: triggers:
Line 105: notbuilt: true
Line 106:
Line 107:
Line 108: - trigger:
Line 109: name: on-patch-positive-code-review
Maybe just call it on-code-review, as it's actually triggered on comment with
code review, not patch. Maybe even better to use on-approved?
Line 110: triggers:
Line 111: - gerrit:
Line 112: trigger-on-comment-added-event: true
Line 113: trigger-approval-category: 'CRVW'
--
To view, visit https://gerrit.ovirt.org/37069
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If411d80e4213f5d4a5d85defeb6bb1634f9bcae4
Gerrit-PatchSet: 9
Gerrit-Project: jenkins
Gerrit-Branch: master
Gerrit-Owner: David Caro <[email protected]>
Gerrit-Reviewer: David Caro <[email protected]>
Gerrit-Reviewer: Dima Kuznetsov <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
