David Caro has posted comments on this change. Change subject: reorg repoclosure script for ease of maintenance ......................................................................
Patch Set 1: Code-Review-1 (42 comments) Most of the comments can be allied more than one, just got tired of repeating them :/. Great idea using an array for the repos! (and bats). https://gerrit.ovirt.org/#/c/39249/1/jobs/packaging/repo_closure_check.sh File jobs/packaging/repo_closure_check.sh: Line 18: Line 19: LAYOUT="" Line 20: CUSTOM_URL="" Line 21: STATIC_RP="" Line 22: test -z "${TEST_RUN}" && TEST_RUN=0 Use bash internal conditional '[[ ]]' Don't use an environment variable, use a parameter instead. Line 23: Line 24: function die() { Line 25: local msg="${1}" Line 26: echo "FATAL: ${msg}" Line 20: CUSTOM_URL="" Line 21: STATIC_RP="" Line 22: test -z "${TEST_RUN}" && TEST_RUN=0 Line 23: Line 24: function die() { The portable way to write functions is without the 'function' keyword, and it does not add readability (there are no classes or anything else in bash) http://www.ovirt.org/Bash_style_guide#Function_definitions Line 25: local msg="${1}" Line 26: echo "FATAL: ${msg}" Line 27: exit 1 Line 28: } Line 76: done Line 77: } Line 78: Line 79: function validation() { Line 80: [[ -z "${BASE_URL}" ]] && BASE_URL="http://resources.ovirt.org"; You could use the maybe nicer: BASE_URL="${BASE_URL:-http://resources.ovirt.org}"; Or better, define the globals at the top of the script with the default values. Should be modifiable by parameter, not by env whenever possible, env vars usually lead to unwanted behavior (the can be set and reset in too many ways and places) Better explicit than implicit Line 81: [[ -z "${CENTOS_MIRROR}" ]] && CENTOS_MIRROR="http://centos.mirror.constant.com/"; Line 82: [[ -z "${EPEL_MIRROR}" ]] && EPEL_MIRROR="http://mirror.switch.ch/ftp/mirror"; Line 83: [[ -z "${FEDORA_MIRROR}" ]] && FEDORA_MIRROR="http://mirrors.kernel.org/"; Line 84: [[ -z "${GLUSTER_MIRROR}" ]] && GLUSTER_MIRROR="http://download.gluster.org"; Line 93: Line 94: function repo_data_to_arr(){ Line 95: local delim="${1:-,}" Line 96: shift Line 97: local repo_data=( ${*//${delim}/ } ) Here it does not matter, but usually '*' will not do what you expect, while a quoted '@' will: a=( 1 "2 3" ) for i in ${a[*]}; do echo "$i"; done for i in "${a[*]}"; do echo "$i"; done for i in ${a[@]}; do echo "$i"; done for i in "${a[@]}"; do echo "$i"; done Line 98: echo "${repo_data[@]}" Line 99: Line 100: } Line 101: Line 98: echo "${repo_data[@]}" Line 99: Line 100: } Line 101: Line 102: function repo_data_to_conf_section(){ You don't have to create any repo files, repoclosure can take them as a parameter. And you would not need to parse them as they are already in the format repoclosure needs them. I don't expect us to have so many repo dependencies to exhaust the parameter stack. That would be wrong for a lot of reasons :/ Line 103: local rpath=$1 Line 104: local repoid=$2 Line 105: local field=$3 Line 106: local value=$4 Line 104: local repoid=$2 Line 105: local field=$3 Line 106: local value=$4 Line 107: ## append data: Line 108: if [ ! -f "${rpath}" ]; then Use bash builtin [[ ]] Also put the negation outside the brackets: ! [[ ]] Line 109: touch "${rpath}" Line 110: fi Line 111: cat >> ${rpath} << EOF Line 112: [${repoid}] Line 107: ## append data: Line 108: if [ ! -f "${rpath}" ]; then Line 109: touch "${rpath}" Line 110: fi Line 111: cat >> ${rpath} << EOF quotes Line 112: [${repoid}] Line 113: name=${repoid} Line 114: ${field}=${value} Line 115: enabled=1 Line 121: function gen_yum_conf() { Line 122: local delim="${1:-,}" Line 123: local yumconfdir="${2}" Line 124: if [[ -z "${yumconfdir}" ]]; then Line 125: yumconfdir=$( mktemp -d tmpyumconfdir.XXXXXXXX ) you should add a cleanup for this using trap, if you use temp dirs Line 126: fi Line 127: local conffile=${yumconfdir}/yum.conf Line 128: local logfile=${yumconfdir}/yum.log Line 129: local reposdir=${yumconfdir}/yum.repos.d Line 126: fi Line 127: local conffile=${yumconfdir}/yum.conf Line 128: local logfile=${yumconfdir}/yum.log Line 129: local reposdir=${yumconfdir}/yum.repos.d Line 130: cat > ${conffile} << EOF quotes Line 131: [main] Line 132: cachedir=/var/cache/yum/\$basearch/\$releasever Line 133: keepcache=0 Line 134: debuglevel=2 Line 146: Line 147: function append_repos_to_rpath() { Line 148: local delim rpath reposdir repos Line 149: delim=$1 Line 150: shift Don't use this shift, it masks the second parameter. Add a comment explaining the parameters it accepts and such, that will help debug later on Line 151: rpath=$1 Line 152: shift Line 153: repos=( $* ) Line 154: reposdir=$( dirname "${rpath}" ) Line 149: delim=$1 Line 150: shift Line 151: rpath=$1 Line 152: shift Line 153: repos=( $* ) Use "$@" instead, to preserve white spaces correctly Line 154: reposdir=$( dirname "${rpath}" ) Line 155: mkdir -p "${reposdir}" Line 156: for repo in ${repos[@]}; do Line 157: repo_data_to_conf_section ${rpath} $( repo_data_to_arr ${delim} $repo ) Line 152: shift Line 153: repos=( $* ) Line 154: reposdir=$( dirname "${rpath}" ) Line 155: mkdir -p "${reposdir}" Line 156: for repo in ${repos[@]}; do use quotes to preserve white spaces: "${repos[@]}" Line 157: repo_data_to_conf_section ${rpath} $( repo_data_to_arr ${delim} $repo ) Line 158: done Line 159: } Line 160: Line 153: repos=( $* ) Line 154: reposdir=$( dirname "${rpath}" ) Line 155: mkdir -p "${reposdir}" Line 156: for repo in ${repos[@]}; do Line 157: repo_data_to_conf_section ${rpath} $( repo_data_to_arr ${delim} $repo ) Use quotes, and maybe split in multilines: repo_data_to_conf_section \ "$rpath" \ $(repo_data_to_arr "$delim" "$repo") Line 158: done Line 159: } Line 160: Line 161: Line 162: function make_yum_config() { Line 163: local delim repos yumconfdir conffile rpath Line 164: delim="${1:-,}" Line 165: shift Line 166: repos=( $* ) Use quoted "$@" Line 167: yumconfdir=$( gen_yum_conf ${delim} ) Line 168: conffile=${yumconfdir}/yum.conf Line 169: rpath=${reposdir}/repos.repo Line 170: append_repos_to_rpath ${delim} ${rpath} ${repos[@]} Line 173: Line 174: function gen_lookaside_repos_list() { Line 175: local delim="${1:-,}" Line 176: shift Line 177: local repos=( $* ) use "$@" Line 178: local result="" Line 179: for repo in ${repos[@]}; do Line 180: repo_data=( $( repo_data_to_arr ${delim} ${repo} ) ) Line 181: repoid=${repo_data[0]} Line 176: shift Line 177: local repos=( $* ) Line 178: local result="" Line 179: for repo in ${repos[@]}; do Line 180: repo_data=( $( repo_data_to_arr ${delim} ${repo} ) ) missing local, and quotes around $delim and $repo Line 181: repoid=${repo_data[0]} Line 182: result="${result} --lookaside ${repoid}" Line 183: done Line 184: echo "${result}" Line 177: local repos=( $* ) Line 178: local result="" Line 179: for repo in ${repos[@]}; do Line 180: repo_data=( $( repo_data_to_arr ${delim} ${repo} ) ) Line 181: repoid=${repo_data[0]} same here Line 182: result="${result} --lookaside ${repoid}" Line 183: done Line 184: echo "${result}" Line 185: } Line 178: local result="" Line 179: for repo in ${repos[@]}; do Line 180: repo_data=( $( repo_data_to_arr ${delim} ${repo} ) ) Line 181: repoid=${repo_data[0]} Line 182: result="${result} --lookaside ${repoid}" and here Line 183: done Line 184: echo "${result}" Line 185: } Line 186: Line 282: fi Line 283: } Line 284: Line 285: function check_repo_closure_new() { Line 286: local distid="${DISTRIBUTION}${DISTRIBUTION_VERSION}" I know it was not following this before, but avoid using global variables, if possible replace by parameters Line 287: local repos=() Line 288: case ${DISTRIBUTION} in Line 289: el|Centos) Line 290: # TODO: extract these into files? Line 290: trailing spaces Line 291: repos+=( : "check-custom-${distid},baseurl,${CUSTOM_URL}" : "check-base-${distid},baseurl,${CENTOS_MIRROR}/${DISTRIBUTION_VERSION}/os/x86_64/" : "check-updates-${distid},baseurl,${CENTOS_MIRROR}/${DISTRIBUTION_VERSION}/updates/x86_64/" : "check-extras-${distid},baseurl,${CENTOS_MIRROR}/${DISTRIBUTION_VERSION}/extras/x86_64/" : "check-epel-${distid},baseurl,${EPEL_MIRROR}/epel/${DISTRIBUTION_VERSION}/x86_64/" : "check-glusterfs-epel-${distid}-nightly,baseurl,${GLUSTER_MIRROR}/pub/gluster/glusterfs/nightly/glusterfs/epel-${DISTRIBUTION_VERSION}-x86_64/" : "check-glusterfs-epel-${distid},baseurl,${GLUSTER_MIRROR}/pub/gluster/glusterfs/LATEST/EPEL.repo/epel-${DISTRIBUTION_VERSION}/x86_64/" : "check-glusterfs-epel-noarch-${distid},baseurl,${GLUSTER_MIRROR}/pub/gluster/glusterfs/LATEST/EPEL.repo/epel-${DISTRIBUTION_VERSION}/noarch/" : "check-patternfly-${distid},baseurl,${COPR}/patternfly/patternfly1/epel-${DISTRIBUTION_VERSION}-x86_64/" : ) : : if [[ "${DISTRIBUTION_VERSION}" != "7" ]]; then : repos+=( : "check-jpackage-generic-${distid},baseurl,${JPACKAGE_MIRROR}/jpackage/6.0/generic/free" : ) : fi lacks one indentation level. case $what in match) do whatever ;; *) default ;; esac Line 306: ) Line 307: fi Line 308: ;; Line 309: fc|Fedora) Line 310: # TODO: extract these into files? Maybe start declaring them as globals for easy access. Line 311: repos+=( Line 312: "check-custom-${distid},baseurl,${CUSTOM_URL}" Line 313: "check-fedora-${distid},baseurl,${FEDORA_MIRROR}/fedora/releases/${DISTRIBUTION_VERSION}/Everything/x86_64/os/" Line 314: "check-updates-${distid},baseurl,${FEDORA_MIRROR}/fedora/updates/${DISTRIBUTION_VERSION}/x86_64/" Line 328: static_url missing local Line 325: Line 326: # TODO: extract these into data files? Line 327: if [[ -n "${STATIC_REPO}" ]]; then Line 328: static_url="${BASE_URL}/${STATIC_REPO}/rpm/${repo}" Line 329: repoid="check-custom-static-$distver" same here Line 330: repos+=( "${repoid},baseurl,${static_url}" ) Line 331: fi Line 332: Line 333: yum_config=$( make_yum_config "," ${repos[@]} ) Line 329: repoid="check-custom-static-$distver" Line 330: repos+=( "${repoid},baseurl,${static_url}" ) Line 331: fi Line 332: Line 333: yum_config=$( make_yum_config "," ${repos[@]} ) quotes: "${repos[@]}" Line 334: look_aside=$( gen_lookaside_repos_list "," ${repos[@]:1} ) Line 335: check_repo=$( repo_data_to_arr "," ${repos[0]} ) Line 336: repoclosure --tempcache --conf ${yum_config} ${look_aside} -r ${check_repo} Line 337: } Line 330: repos+=( "${repoid},baseurl,${static_url}" ) Line 331: fi Line 332: Line 333: yum_config=$( make_yum_config "," ${repos[@]} ) Line 334: look_aside=$( gen_lookaside_repos_list "," ${repos[@]:1} ) quotes Line 335: check_repo=$( repo_data_to_arr "," ${repos[0]} ) Line 336: repoclosure --tempcache --conf ${yum_config} ${look_aside} -r ${check_repo} Line 337: } Line 338: Line 331: fi Line 332: Line 333: yum_config=$( make_yum_config "," ${repos[@]} ) Line 334: look_aside=$( gen_lookaside_repos_list "," ${repos[@]:1} ) Line 335: check_repo=$( repo_data_to_arr "," ${repos[0]} ) local local local, and quotes Line 336: repoclosure --tempcache --conf ${yum_config} ${look_aside} -r ${check_repo} Line 337: } Line 338: Line 339: function main() { Line 332: Line 333: yum_config=$( make_yum_config "," ${repos[@]} ) Line 334: look_aside=$( gen_lookaside_repos_list "," ${repos[@]:1} ) Line 335: check_repo=$( repo_data_to_arr "," ${repos[0]} ) Line 336: repoclosure --tempcache --conf ${yum_config} ${look_aside} -r ${check_repo} Split in multline, one <option[+argument]|argument> per line: repoclosure \ --tempcache \ --conf "$yum_config" \ $look_aside \ -r "$check_repo" Line 337: } Line 338: Line 339: function main() { Line 340: get_opts "${@}" Line 344: # check_repo_closure Line 345: check_repo_closure_new Line 346: } Line 347: Line 348: if [ ${TEST_RUN} -eq 0 ]; then [[ ]] Line 349: main "${@}" Line 350: RESULT=$? Line 351: exit ${RESULT} Line 347: Line 348: if [ ${TEST_RUN} -eq 0 ]; then Line 349: main "${@}" Line 350: RESULT=$? Line 351: exit ${RESULT} no need for RESULT here https://gerrit.ovirt.org/#/c/39249/1/jobs/packaging/test_repo_closure_check.sh File jobs/packaging/test_repo_closure_check.sh: Line 7: local msg="${1}" Line 8: echo "FATAL: ${msg}" Line 9: } Line 10: Line 11: @test "test function: validation" { No need to repeat the test in the name again, it's already clear Line 12: Line 13: DISTRIBUTION="Ubuntu" Line 14: REPO_NAME="kuku" Line 15: DISTRIBUTION_VERSION="99" Line 17: trailing whitespace Line 33: Line 34: unset REPO_NAME Line 35: actual=$( validation ) Line 36: errmsg="Please specify --repo= option" Line 37: expected="FATAL: ${errmsg}" Error messages should be outed to stderr, and the function should return != 0. Take into account that bats will fail when any of the commands fail, meaning that: res=$(fail) [[ "$res" == "ERROR" ]] Will fail before testing the output, you can use the bats-specific run function, that will wrap the execution and set a few env vars like output (check the help, don't remember the exact names) Line 38: [ "${expected}" = "${actual}" ] Line 39: unset actual expected errmsg Line 40: } Line 41: 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}" ] use bash builtin '[[]]' Line 39: unset actual expected errmsg Line 40: } Line 41: Line 42: @test "test function: repo_data_to_arr" { Line 35: actual=$( validation ) Line 36: errmsg="Please specify --repo= option" Line 37: expected="FATAL: ${errmsg}" Line 38: [ "${expected}" = "${actual}" ] Line 39: unset actual expected errmsg better use local than unset, though it seems bats cleans up all the vars before each test (tested on my machine) Line 40: } Line 41: Line 42: @test "test function: repo_data_to_arr" { Line 43: raw=("ks44" "kksks" "jfdjf") Line 42: @test "test function: repo_data_to_arr" { Line 43: raw=("ks44" "kksks" "jfdjf") Line 44: delim=" " Line 45: expected="" Line 46: for i in ${raw[@]}; do missing quotes Line 47: if [ -z "${expected}" ]; then Line 48: expected="${i}" Line 49: else Line 50: expected="${expected}${delim}${i}" Line 43: raw=("ks44" "kksks" "jfdjf") Line 44: delim=" " Line 45: expected="" Line 46: for i in ${raw[@]}; do Line 47: if [ -z "${expected}" ]; then [[]] Line 48: expected="${i}" Line 49: else Line 50: expected="${expected}${delim}${i}" Line 51: fi 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 52: done Line 53: delim="|" Line 54: processed="" Line 55: for i in ${raw[@]}; do Line 56: if [ -z "${processed}" ]; then [[]] Line 57: processed="${i}" Line 58: else Line 59: processed="${processed}${delim}${i}" Line 60: fi 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}" ] Use bash [[]], and double == Line 64: Line 65: expected="" Line 66: actual=$( repo_data_to_arr ${delim} ) Line 67: [ "${expected}" = "${actual}" ] Line 81: field="mirrorlist" Line 82: value="http://kuku.com/alsdkas/kasd"; Line 83: repo_data_to_conf_section ${rpath} ${repoid} ${field} ${value} Line 84: diff ${rpath_fixture} ${rpath} &> /dev/null Line 85: [ $? -eq 0 ] This is not needed, bats will fail if diff fails. Also not redirecting to /dev/null might help debugging, as bats will show the output in case it fails only Line 86: rm -f ${rpath} Line 87: unset rpath rpath_fixture repoid field value Line 88: } Line 89: Line 115: append_repos_to_rpath ${delim} ${rpath} ${repos[@]} Line 116: [ -s "${rpath}" ] Line 117: Line 118: diff ${rpath_fixture} ${rpath} &> /dev/null Line 119: [ $? -eq 0 ] no need Line 120: rm -f ${rpath} Line 121: unset delim confroot_fixture rpath_fixture reposdir rpath repos Line 122: } Line 123: -- 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: 1 Gerrit-Project: jenkins Gerrit-Branch: master Gerrit-Owner: Max Kovgan <[email protected]> Gerrit-Reviewer: David Caro <[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
