Patch Set 4: Code-Review-1 (4 comments)
I'd love to +2 to move ahead, but I find it really hard to follow what the code is doing. Half the bash functions called are assumed to be defined somewhere else, while the link from the commit log shows me that those again should call back into bash functions defined here. This needs to be made simpler, or it needs very thorough in-code documentation defining what "users" of these scripts must provide. Why are these two separate scripts when one sources the other anyway? Will sourcing the other not pose a problem when this one was itself sourced from a different directory? >From skimming along, I couldn't figure out where the dependencies would >actually be built... Could you collapse the code and/or clarify, please? (Best by in-code comments so that the docs are not "lost" for later readers of the code, and maybe by a more obvious call chain) I hope it's not because I'm too dense to see the truth in this patch... General style: we have CamelCase for python/C++ classes, but that's about it. Otherwise all code uses underscored_names AFAICT. I find it too Java-like to have LongFunctionNamesInCamelCase :) ... but YMMV, I would still accept the patch despite the naming style. https://gerrit.osmocom.org/#/c/2465/4/scripts/osmo-artifacts.sh File scripts/osmo-artifacts.sh: Line 5: genericDeps "getBranchAndRevByLocalRepo" where is genericDeps defined? Ah, it comes from that "external" script https://github.com/blobbsen/diy-artifacts/blob/master/jenkins-openBSC.sh Line 61: generateArtifactHashes() { so this function writes various checksums to stdout -- are these just for the log or actually used somewhere? Line 70: sleep 1 why sleep? https://gerrit.osmocom.org/#/c/2465/4/scripts/osmo-build.sh File scripts/osmo-build.sh: Line 62: buildProject where is buildProject defined? -- also in the "external" script -- To view, visit https://gerrit.osmocom.org/2465 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifee0a2f837d23b19aa5326f810234d5452e47484 Gerrit-PatchSet: 4 Gerrit-Project: osmo-ci Gerrit-Branch: master Gerrit-Owner: blobb <dr.bl...@gmail.com> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Holger Freyther <hol...@freyther.de> Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
