David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/11128 )
Change subject: IMPALA-7399: Add script in lib/python to generate junit XML. ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/11128/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11128/2//COMMIT_MSG@11 PS2, Line 11: python library for Impala development that can be pip installed into > You're going to update bootstrap* in a separate commit? This doesn't curren I think we have some options with regard to usage. I was undecided about which to pursue. I can see doing one of two things. One option would be to set up a separate parallel virtualenv purely for doing jenkinsy things in our Jenkins scripts, and call this explicitly. $ virtualenv jenkins_venv New python executable in /home/<user>/jenkins_venv/bin/python Installing setuptools, pip, wheel...done. $ ./jenkins_venv/bin/pip install -e $IMPALA_HOME/lib/python [...snip...] Successfully installed impala-py-lib junit-xml-1.8 pytz-2018.5 six-1.11.0 $ ./jenkins_venv/bin/python $IMPALA_HOME/lib/python/impala_py_lib/jenkins/generate-junitxml.py usage: generate-junitxml.py [-h] [--phase PHASE] --step STEP [-t TIME] [--stdout STDOUT] [--stderr STDERR] [--error ERROR] generate-junitxml.py: error: argument --step is required The other option would be to install stuff directly into the Impala python infra env, but you're right -- we'd run the risk of failing before this script could be employed. I realize that the given example might feel a bit cumbersome, but it does protect us from failing in the build before bootstrap runs, and it gives access to this script from the very start. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py File lib/python/jenkins/impala_junitxml_logger.py: http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@1 PS2, Line 1: #!/usr/bin/env python > Bike shed on the naming of "jenkins_junitxml_logger.py" I think you meant impala_junitxml_logger.py? In any event, done. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@36 PS2, Line 36: # The equivalent of mkdir -p : try: : os.makedirs(JUNITXML_LOGDIR) : except OSError as e: : if e.errno == errno.EEXIST and os.path.isdir(JUNITXML_LOGDIR): : pass : else: : raise > Please put this into main(). Done http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@60 PS2, Line 60: with open(file_or_string, 'r') as f: : content = f.read() > This is shorter: True. I tend to slavishly follow the convention of always wrapping calls to open() within a with-block, even though I know it doesn't matter in this case because the process ends immediately anyway. If you feel strongly, I'll change this. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@82 PS2, Line 82: raise OSError("File already exists: {}".format(junit_log_file)) > There are scenarios where this is too picky. When I run unit tests over and OK, I'll generate a new file. See my reply to another comment for an explanation as to why I decided to enforce uniqueness. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@97 PS2, Line 97: help="General build phase or script [default = buildall.") > nit: [default = buildall]. Done http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@107 PS2, Line 107: help="If known, the amount of elapsed time for this step.") > Mention that this is in seconds. Done http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@112 PS2, Line 112: either a plain string, or the path to a file print out.""") > nit: "Can be either a string or the path to a file." Hmm. That looks like a copy paste error. Thanks for pointing it out. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@140 PS2, Line 140: equivalent to a "test case", must be unique. > If an output file already exists because this is invalid, do you want to cr Not useful in the general Jenkins context, but I think failing the script is useful when *adding* new junit xml output to the build sequence. For example: Patch 1. Someone decides we want to generate a junit step for step foo during the data load phase. Patch 2. Later on, someone else decides to add junit xml output elsewhere during data load for step bar. When testing patch 2 before submitting a review, if there was a copy/paste error in the name of the step, or if both step names were poorly chosen (e.g., maybe too generic, like "load"), then the second patch should fail to force that person to change the name of the step. Otherwise, the first file could be silently overwritten, and we may not realize until we need it to triage some build. http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@160 PS2, Line 160: xml_report = generate_xml_file(testsuite) > I would pass in the root directory here too instead of having a global. You Done http://gerrit.cloudera.org:8080/#/c/11128/2/lib/python/jenkins/impala_junitxml_logger.py@161 PS2, Line 161: print("Generated: {}".format(xml_report)) > Does this need a "from future import print_function" thingy? Turns out, no. (I was surprised too.) I should probably confirm that's the case on python 2.6 as well. -- To view, visit http://gerrit.cloudera.org:8080/11128 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6024d74075ea69b8ee20d1fc3cc9c1ff821ba5b Gerrit-Change-Number: 11128 Gerrit-PatchSet: 3 Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Mon, 06 Aug 2018 21:43:13 +0000 Gerrit-HasComments: Yes
