David Caro has posted comments on this change. Change subject: reorg repoclosure script for ease of maintenance ......................................................................
Patch Set 4: (21 comments) https://gerrit.ovirt.org/#/c/39249/4/jobs/packaging/repo_closure_check.sh File jobs/packaging/repo_closure_check.sh: Line 332: # $1 - distro: distro short name Line 333: # $2 - distro_ver: distro version Line 334: # TODO: stop using env variables, i.e. pass mirrors are params Line 335: Line 336: local distro distro_ver distid static_url repoid yum_config look_aside check_repo yumconfdir I don't like this :/, maybe splitting into more lines: local distro distro_ver local distid static_url local repoid yum_config local look_aside check_repo local yumconfdir what about one per line like this? local distro \ distro_ver \ distid \ static_url \ repoid \ yum_config \ look_aside \ check_repo \ yumconfdir Seeing it written I vote this one, a lot more readable! Line 337: local repos=() Line 338: distro="${1:-$DISTRIBUTION}" Line 339: distro_ver="${2:-$DISTRIBUTION_VERSION}" Line 340: distid="${distro}${distro_ver}" Line 341: Line 342: case ${DISTRIBUTION} in Line 343: el|Centos) Line 344: # TODO: extract these into files? Line 345: repos+=( lacking indentation level Line 346: "check-custom-${distid},baseurl,${CUSTOM_URL}" Line 347: "check-base-${distid},baseurl,${CENTOS_MIRROR}/${distro_ver}/os/x86_64/" Line 348: "check-updates-${distid},baseurl,${CENTOS_MIRROR}/${distro_ver}/updates/x86_64/" Line 349: "check-extras-${distid},baseurl,${CENTOS_MIRROR}/${distro_ver}/extras/x86_64/" Line 403: # check_repo_closure Line 404: check_repo_closure_new "${DISTRIBUTION}" "${DISTRIBUTION_VERSION}" Line 405: } Line 406: Line 407: if [[ ${TEST_RUN} -eq 0 ]]; then Maybe add a small echo or something at least, so you know it ran. At best show the repoman command without executing it and the extracted urls Line 408: main "${@}" Line 409: exit $? https://gerrit.ovirt.org/#/c/39249/4/jobs/packaging/test_repo_closure_check.sh File jobs/packaging/test_repo_closure_check.sh: Line 1: #!/usr/bin/env bats Line 2: Line 3: TEST_RUN=1 So this is what the var is for... we might want to differentiate between unit and functional tests somehow. I think that it's better to add to the script: if ! [[ "$0" =~ /bash$ ]]; then main fi than the TEST_RUN env var, like python's if __name__ == "__main__".. I think I'll propose it for the style guide :) Line 4: source ./repo_closure_check.sh Line 5: Line 6: function die(){ Line 7: local msg="${1}" Line 17: still trailing whitespaces Line 28: [ ! -z "${EPEL_MIRROR}" ] Line 29: [ ! -z "${FEDORA_MIRROR}" ] Line 30: [ ! -z "${GLUSTER_MIRROR}" ] Line 31: [ ! -z "${JPACKAGE_MIRROR}" ] Line 32: [ ! -z "${COPR}" ] [[]] * 7 Line 33: Line 34: unset REPO_NAME Line 35: actual=$( validation ) Line 36: errmsg="Please specify --repo= option" Line 34: unset REPO_NAME Line 35: actual=$( validation ) Line 36: errmsg="Please specify --repo= option" Line 37: expected="FATAL: ${errmsg}" Line 38: [ "${expected}" = "${actual}" ] [[ ]] Line 39: } Line 40: Line 41: @test "test function: repo_data_to_arr" { Line 42: local raw delim expected processed actual Line 42: local raw delim expected processed actual Line 43: raw=("ks44" "kksks" "jfdjf") Line 44: delim=" " Line 45: expected="" Line 46: for i in ${raw[@]}; do quotes Line 47: if [[ -z "${expected}" ]]; then Line 48: expected="${i}" Line 49: else Line 50: expected="${expected}${delim}${i}" Line 51: fi Line 52: done Line 53: delim="|" Line 54: processed="" Line 55: for i in ${raw[@]}; do quotes Line 56: if [ -z "${processed}" ]; then Line 57: processed="${i}" Line 58: else Line 59: processed="${processed}${delim}${i}" Line 56: [[]] Line 59: processed="${processed}${delim}${i}" Line 60: fi Line 61: done Line 62: actual=$( repo_data_to_arr "${delim}" "${processed}" ) Line 63: [ "${expected}" = "${actual}" ] [[]] Line 64: Line 65: expected="" Line 66: actual=$( repo_data_to_arr "${delim}" ) Line 67: [ "${expected}" = "${actual}" ] Line 63: [ "${expected}" = "${actual}" ] Line 64: Line 65: expected="" Line 66: actual=$( repo_data_to_arr "${delim}" ) Line 67: [ "${expected}" = "${actual}" ] [[]] Line 68: Line 69: expected="ksk" Line 70: actual=$( repo_data_to_arr "${delim}" "${expected}" ) Line 71: [ "${expected}" = "${actual}" ] Line 67: [ "${expected}" = "${actual}" ] Line 68: Line 69: expected="ksk" Line 70: actual=$( repo_data_to_arr "${delim}" "${expected}" ) Line 71: [ "${expected}" = "${actual}" ] [[]] Line 72: } Line 73: Line 74: Line 75: @test "test function: repo_data_to_conf_section" { Line 81: repoid="kuku" Line 82: field="mirrorlist" Line 83: value="http://kuku.com/alsdkas/kasd"; Line 84: repo_data_to_conf_section "${rpath}" "${repoid}" "${field}" "${value}" Line 85: diff ${rpath_fixture} ${rpath} &> /dev/null you don't have to redirect here, the output will only be shown on failure Line 86: [ $? -eq 0 ] Line 87: rm -f ${rpath} Line 88: Line 89: } Line 82: field="mirrorlist" Line 83: value="http://kuku.com/alsdkas/kasd"; Line 84: repo_data_to_conf_section "${rpath}" "${repoid}" "${field}" "${value}" Line 85: diff ${rpath_fixture} ${rpath} &> /dev/null Line 86: [ $? -eq 0 ] no need for this, it will fail if diff fails Line 87: rm -f ${rpath} Line 88: Line 89: } Line 90: Line 89: } Line 90: Line 91: @test "test function: gen_yum_conf" { Line 92: local confroot_fixture conf_file_fixture myconfroot Line 93: confroot_fixture="./yum_conf" double check that the using relative paths here is ok, there are some env vars te get tests dir and such Line 94: conf_file_fixture="${confroot_fixture}/fixture_yum.conf" Line 95: myconfroot=$( gen_yum_conf "${confroot_fixture}" ) Line 96: [ "${myconfroot}" = "${confroot_fixture}" ] Line 97: Line 92: local confroot_fixture conf_file_fixture myconfroot Line 93: confroot_fixture="./yum_conf" Line 94: conf_file_fixture="${confroot_fixture}/fixture_yum.conf" Line 95: myconfroot=$( gen_yum_conf "${confroot_fixture}" ) Line 96: [ "${myconfroot}" = "${confroot_fixture}" ] [[]] Line 97: Line 98: diff "${conf_file_fixture}" "${myconfroot}/yum.conf" &> /dev/null Line 99: [ $? -eq 0 ] Line 100: rm -f "${myconfroot}/yum.conf" Line 95: myconfroot=$( gen_yum_conf "${confroot_fixture}" ) Line 96: [ "${myconfroot}" = "${confroot_fixture}" ] Line 97: Line 98: diff "${conf_file_fixture}" "${myconfroot}/yum.conf" &> /dev/null Line 99: [ $? -eq 0 ] no need for this, it will fail if diff fails Line 100: rm -f "${myconfroot}/yum.conf" Line 101: } Line 102: Line 103: Line 113: "kuakua,baseurl,http://kuakua.com/alsdkas/kasd/jdjdj"; Line 114: ) Line 115: Line 116: append_repos_to_rpath "${delim}" "${rpath}" "${repos[@]}" Line 117: [ -s "${rpath}" ] [[]] Line 118: Line 119: diff "${rpath_fixture}" "${rpath}" &> /dev/null Line 120: [ $? -eq 0 ] Line 121: rm -f "${rpath}" Line 116: append_repos_to_rpath "${delim}" "${rpath}" "${repos[@]}" Line 117: [ -s "${rpath}" ] Line 118: Line 119: diff "${rpath_fixture}" "${rpath}" &> /dev/null Line 120: [ $? -eq 0 ] no need for this, it will fail if diff fails Line 121: rm -f "${rpath}" Line 122: rm -fr "${reposdir}" Line 123: } Line 124: Line 130: "fjfj,ieieie,http://fjfj.com/ksks"; Line 131: ) Line 132: expected=" --lookaside quaqua --lookaside fjfj" Line 133: actual=$( gen_lookaside_repos_list "${delim}" "${repos[@]}" ) Line 134: [ "${expected}" = "${actual}" ] [[]] -- 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: 4 Gerrit-Project: jenkins Gerrit-Branch: master Gerrit-Owner: Max Kovgan <[email protected]> Gerrit-Reviewer: David Caro <[email protected]> Gerrit-Reviewer: Max Kovgan <[email protected]> Gerrit-Reviewer: Sandro Bonazzola <[email protected]> Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
