David Caro has posted comments on this change. Change subject: reorg repoclosure + tests ......................................................................
Patch Set 30: (13 comments) https://gerrit.ovirt.org/#/c/39249/30/jobs/packaging/repo_closure_check.sh File jobs/packaging/repo_closure_check.sh: Line 50: ## on test run we don't want to cleanup automatically Line 51: break Line 52: fi Line 53: echo "rm_on_exit running: rm -fr ${item}" Line 54: [[ -e "${item}" ]] && rm -fr "${item}" rm -rf will only fail if it has no permission, the existence or not if the dest is irrelevant (does not hurt, but it's useless) Line 55: done Line 56: } Line 57: Line 58: add_rm_on_exit() Line 54: [[ -e "${item}" ]] && rm -fr "${item}" Line 55: done Line 56: } Line 57: Line 58: add_rm_on_exit() Remove all this functions/globals tricky calls, you only use it once, there's no need to get complicated That also removes the need for the TEST_RUN global too Line 59: { Line 60: MY_TMP_TRASH+=("${@}") Line 61: trap rm_on_exit EXIT Line 62: } Line 125: validation() { Line 126: local \ Line 127: distro \ Line 128: layout \ Line 129: dlm \ the other name is a lot better Line 130: message \ Line 131: ok Line 132: [[ -n "${DISTRIBUTION}" ]] || die $? "Please specify --distribution= option" Line 133: [[ -n "${REPO_NAME}" ]] || die $? "Please specify --repo= option" Line 133: [[ -n "${REPO_NAME}" ]] || die $? "Please specify --repo= option" Line 134: [[ -n "${DISTRIBUTION_VERSION}" ]] || die $? "Please specify --distribution-version= option" Line 135: [[ -n "${LAYOUT}" ]] || die $? "Please specify --layout= option" Line 136: ok=0 Line 137: # dlm - distro -> layout mapping if you kept the name there would be no need for comments Line 138: for dlm in "${DISTRO_LAYOUTS[@]}"; do Line 139: distro="${dlm%%:*}" Line 140: layout="${dlm#*:}" Line 141: [[ "${distro}" == "${DISTRIBUTION}" ]] || continue Line 155: local \ Line 156: delim \ Line 157: repo_data_raw \ Line 158: repo_data Line 159: delim="${1:-,}" do you really have to pass around the delimiter? you expect changing it at any point? does the extra logic/complications pay for the ease of being able to pass it as a parameter? Line 160: repo_data_raw=( "${@:2}" ) Line 161: repo_data=( "${repo_data_raw[@]//${delim}/ }" ) Line 162: echo "${repo_data[@]}" Line 163: Line 189: echo "[${repoid}]" >> "${rpath}" Line 190: echo "name=${repoid}" >> "${rpath}" Line 191: echo "${field}=${value}" >> "${rpath}" Line 192: echo "enabled=1" >> "${rpath}" Line 193: echo "gpgcheck=0" >> "${rpath}" you could ease a bit this wich something like: echo -e \ "line one\n" \ "line two\n" \ ... >> "$rpath" Line 194: [[ -n "${extras}" ]] && echo -e "${extras}" >> "${rpath}" Line 195: echo "" >> "${rpath}" Line 196: } Line 197: Line 505: *repo_closure_check.sh this does not look like a regex, does it work? https://gerrit.ovirt.org/#/c/39249/30/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 not needed Line 4: tested="repo_closure_check" Line 5: [[ -h "${tested}.bash" ]] || ln -s "${tested}.sh" "${tested}.bash" Line 6: load "${tested}" Line 7: Line 2: Line 3: TEST_RUN=1 Line 4: tested="repo_closure_check" Line 5: [[ -h "${tested}.bash" ]] || ln -s "${tested}.sh" "${tested}.bash" Line 6: load "${tested}" what is this for? Line 7: Line 8: Line 9: @test "test function: validation" { Line 10: local \ Line 39: load "${tested}" Line 40: run validation Line 41: Line 42: expected="FATAL: Please specify --repo= option" Line 43: [[ 0 -eq "${status}" ]] shouldn't it return 1? Line 44: [[ "${expected}" == "${lines[0]}" ]] Line 45: Line 46: Line 47: DISTRIBUTION="Ubuntu" Line 79: fi Line 80: done Line 81: delim="|" Line 82: processed="" Line 83: for i in ${raw[@]}; do quotes Line 84: if [[ -z "${processed}" ]]; then Line 85: processed="${i}" Line 86: else Line 87: processed="${processed}${delim}${i}" Line 121: extras="exclude=mesdb*" Line 122: run repo_data_to_conf_section "${rpath}" "${repoid}" "${field}" "${value}" "${extras}" Line 123: [[ 0 -eq "${status}" ]] Line 124: [[ "" == "${output}" ]] Line 125: run diff ${rpath_fixture} ${rpath} &> /dev/null no need to redirect Line 126: [[ 0 -eq "${status}" ]] Line 127: rm -f "${rpath}" Line 128: } Line 129: Line 159: "kuku,mirrorlist,http://kuku.com/alsdkas/kasd,include=libsbonazzo*"; Line 160: "kuakua,baseurl,http://kuakua.com/alsdkas/kasd/jdjdj"; Line 161: ) Line 162: Line 163: run append_repos_to_rpath ${rpath} ${delim} ${repos[@]} quotes Line 164: [[ "" == "${output}" ]] Line 165: run diff "${rpath_fixture}" "${rpath}" &> /dev/null Line 166: [[ 0 -eq "${status}" ]] Line 167: rm -fr "${rpath}" -- 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: 30 Gerrit-Project: jenkins Gerrit-Branch: master Gerrit-Owner: Max Kovgan <m...@redhat.com> Gerrit-Reviewer: David Caro <dcaro...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Max Kovgan <mkov...@redhat.com> Gerrit-Reviewer: Max Kovgan <m...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@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
