David Caro has posted comments on this change. Change subject: reorg repoclosure + tests ......................................................................
Patch Set 14: Code-Review-1 (17 comments) https://gerrit.ovirt.org/#/c/39249/14/jobs/packaging/repo_closure_check.sh File jobs/packaging/repo_closure_check.sh: Line 28: # TEST_RUN is used by test_* script. it avoids running main, thus making Line 29: # possible to test functions defined in this file. Line 30: # another side effect is: die() is becoming undying - for testing purposes Line 31: [[ -z "${TEST_RUN}" ]] && TEST_RUN=0 Line 32: declare -a on_exit_items use all caps for globals, or avoid them as much as possible Line 33: #logfile="running.log" Line 34: #touch $logfile Line 35: #hr="=============================================================================" Line 36: #echo "${hr}" >> $logfile Line 37: #echo "Running stuff now: $( date )" >> $logfile Line 38: #echo "${hr}" >> $logfile Line 39: on_exit() Line 40: { Line 41: for i in "${on_exit_items[@]}" use at least 3 chars for any file Line 42: do Line 43: if [[ ${TEST_RUN} -ne 0 ]]; then Line 44: ## on test run we don't want to cleanup automatically Line 45: break Line 47: eval avoid as much as possible, please give an explanation on why is required here Line 115: done Line 116: } Line 117: Line 118: validation() { Line 119: local d l m o use at least 3 chars for any var Line 120: BASE_URL=${BASE_URL:-"http://resources.ovirt.org"} Line 121: CENTOS_MIRROR=${CENTOS_MIRROR:-"http://centos.mirror.constant.com"} Line 122: EPEL_MIRROR=${EPEL_MIRROR:-"http://mirror.switch.ch/ftp/mirror"} Line 123: FEDORA_MIRROR=${FEDORA_MIRROR:-"http://mirrors.kernel.org"} Line 128: [[ -n "${REPO_NAME}" ]] || die $? "Please specify --repo= option" Line 129: [[ -n "${DISTRIBUTION_VERSION}" ]] || die $? "Please specify --distribution-version= option" Line 130: [[ -n "${LAYOUT}" ]] || die $? "Please specify --layout= option" Line 131: o=0 Line 132: for lm in "${DISTRO_LAYOUTS[@]}"; do lm not local Line 133: d="$( echo "${lm}" | cut -d: -f1 )" Line 134: l="$( echo "${lm}" | cut -d: -f2 )" Line 135: if [[ "${d}" = "${DISTRIBUTION}" ]]; then Line 136: m="Distribution: ${d} uses layout: ${l}" Line 130: [[ -n "${LAYOUT}" ]] || die $? "Please specify --layout= option" Line 131: o=0 Line 132: for lm in "${DISTRO_LAYOUTS[@]}"; do Line 133: d="$( echo "${lm}" | cut -d: -f1 )" Line 134: l="$( echo "${lm}" | cut -d: -f2 )" use internal tools: d="${lm%%:*}" l="${lm#*:}" Line 135: if [[ "${d}" = "${DISTRIBUTION}" ]]; then Line 136: m="Distribution: ${d} uses layout: ${l}" Line 137: [[ "${l}" = "${LAYOUT}" ]] || die 1 "${m}" Line 138: o=1 Line 131: o=0 Line 132: for lm in "${DISTRO_LAYOUTS[@]}"; do Line 133: d="$( echo "${lm}" | cut -d: -f1 )" Line 134: l="$( echo "${lm}" | cut -d: -f2 )" Line 135: if [[ "${d}" = "${DISTRIBUTION}" ]]; then use double equals Line 136: m="Distribution: ${d} uses layout: ${l}" Line 137: [[ "${l}" = "${LAYOUT}" ]] || die 1 "${m}" Line 138: o=1 Line 139: break Line 210: if [[ -z "${yumconfdir}" ]]; then Line 211: #yumconfdir="$( pwd )$( mktemp -d ${prefix}.XXXXXXXX )" Line 212: yumconfdir="$( mktemp -d "${prefix}.XXXXXXXX" )" Line 213: # TODO verify how to work with this trap (if at all) Line 214: # add_on_exit "rm -fr ${yumconfdir}" > /dev/null you can, and you should Line 215: fi Line 216: conffile="${yumconfdir}/yum.conf" Line 217: yumlog="${yumconfdir}/yum.log" Line 218: reposdir="${yumconfdir}/yum.repos.d" Line 488: cmd+=( "repoclosure" ) Line 489: cmd+=( "--tempcache" ) Line 490: cmd+=( "--config=${yum_config}" ) Line 491: cmd+=( "${look_aside}" ) Line 492: cmd+=( "--repoid=${check_repo}" ) you can define it on the same statement: cmd=( "repoclosure" "--tempcache" "--config=..." ... ) not sure why store it on a var at all Line 493: #echo cmd: "${cmd[@]}" Line 494: ${cmd[@]} || die $? "${msg}" Line 495: ## cleanup our mess if it all went well: Line 496: if [ -d "${yumconfdir}" ]; then Line 492: cmd+=( "--repoid=${check_repo}" ) Line 493: #echo cmd: "${cmd[@]}" Line 494: ${cmd[@]} || die $? "${msg}" Line 495: ## cleanup our mess if it all went well: Line 496: if [ -d "${yumconfdir}" ]; then [[ ]] Line 497: rm -fr "${yumconfdir}" Line 498: fi Line 499: } Line 500: Line 506: # check_repo_closure Line 507: check_repo_closure_new "${DISTRIBUTION}" "${DISTRIBUTION_VERSION}" Line 508: } Line 509: Line 510: if [[ ${TEST_RUN} -eq 0 ]]; then Instead of this, check if $0 ends with /bash (that means it has been sourced) Line 511: # only runs when TEST_RUN is 0. Line 512: main "${@}" Line 513: exit $? https://gerrit.ovirt.org/#/c/39249/14/jobs/packaging/test_repo_closure_check.sh File jobs/packaging/test_repo_closure_check.sh: Line 1: #!/usr/bin/env bats use [[ ]] and == everywhere Line 2: Line 3: TEST_RUN=1 Line 4: source repo_closure_check.sh Line 5: Line 21: unset COPR Line 22: ## check non existing vars have been set: Line 23: validation Line 24: [ "${output}" = "" ] Line 25: [ ! -z "${BASE_URL}" ] the ! should be out of the [[ ]] Line 26: [ ! -z "${CENTOS_MIRROR}" ] Line 27: [ ! -z "${EPEL_MIRROR}" ] Line 28: [ ! -z "${FEDORA_MIRROR}" ] Line 29: [ ! -z "${GLUSTER_MIRROR}" ] Line 31: [ ! -z "${COPR}" ] Line 32: Line 33: unset REPO_NAME Line 34: run validation Line 35: [ "${lines[0]}" = "FATAL: Please specify --repo= option" ] [[ ]] Line 36: Line 37: Line 38: DISTRIBUTION="Ubuntu" Line 39: REPO_NAME="kuku" Line 106: field="mirrorlist" Line 107: value="http://kuku.com/alsdkas/kasd"; Line 108: extras="exclude=mesdb*" Line 109: run repo_data_to_conf_section "${rpath}" "${repoid}" "${field}" "${value}" "${extras}" Line 110: [ "${output}" = "" ] you should check status too, it will not fail if using run Line 111: run diff ${rpath_fixture} ${rpath} &> /dev/null Line 112: [ ${status} -eq 0 ] Line 113: rm -f "${rpath}" Line 114: } Line 121: confroot_fixture="./yum_conf" Line 122: conf_file_fixture="${confroot_fixture}/fixture_yum.conf" Line 123: Line 124: run gen_yum_conf ${confroot_fixture} Line 125: [ "${output}" = "${confroot_fixture}" ] same here, check status, and any other occurrence Line 126: Line 127: run diff "${conf_file_fixture}" "${myconfroot}/yum.conf" &> /dev/null Line 128: [ $? -eq 0 ] Line 129: rm -f "${myconfroot}/yum.conf" Line 124: run gen_yum_conf ${confroot_fixture} Line 125: [ "${output}" = "${confroot_fixture}" ] Line 126: Line 127: run diff "${conf_file_fixture}" "${myconfroot}/yum.conf" &> /dev/null Line 128: [ $? -eq 0 ] if you use run, do echo "$output" and check $status instead of $0 Line 129: rm -f "${myconfroot}/yum.conf" Line 130: } Line 131: Line 132: -- To view, visit https://gerrit.ovirt.org/39249 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I207c5c59ca3149c17897c231e391e15e7eec4363 Gerrit-PatchSet: 14 Gerrit-Project: jenkins Gerrit-Branch: master Gerrit-Owner: Max Kovgan <m...@redhat.com> Gerrit-Reviewer: David Caro <dcaro...@redhat.com> Gerrit-Reviewer: Max Kovgan <m...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
