Dinesh Bhat has posted comments on this change. Change subject: KUDU-1566: [scripts] Update jira fields automatically with gerrit link, commit SHA etc ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/4852/2//COMMIT_MSG Commit Message: PS2, Line 9: - > Please wrap the commit message at 72 chars/line Done PS2, Line 12: [1-9] > [1-9]+ Done PS2, Line 14: auo > spelling Done http://gerrit.cloudera.org:8080/#/c/4852/2/src/kudu/scripts/jira_updater.py File src/kudu/scripts/jira_updater.py: PS2, Line 51: re.search(r'\bKUDU-[0-9] > That's a good point, yeah perhaps we can only check if the commit_msg start I resorted to searching in the first commit line, it is prolly reasonable to expect the first line to contain the JIRA to be resolved (and not the one which carries "This doesn't fix KUDU-") PS2, Line 64: update_review_link > If we update the review link, can we also change the status to 'In Review'? I went down that road, apparently 'In Review' follows slightly different workflow than we think - i.e, only transition available from 'In Review' is 'Accept Review'(equivalent to resolve). Hence I decided to update 'Start Progress' in this routine. PS2, Line 66: Update the Jira Comments > Forgot to change the copied docstring Done PS2, Line 73: no better way > Any way to sanity-check it's the right field before we update it? Anyway it I couldn't find a better way unfortunately, hence kept the comment. PS2, Line 74: ''' > Is it PEP8 to have a multiline comment as a string literal? I know python o I didn't know about any PEP standard, I followed an existing script in scripts/ directory, but apparently PEP257 defines that way as per this link: https://www.python.org/dev/peps/pep-0008/ PS2, Line 88: print "update_id %s" %update_id > logging or leftover? if logging, some of the other functions could use logg leftover, thanks for noticing. If there is a need, we could use logging too. PS2, Line 90: If > One line comment should use # Done PS2, Line 93: 'id': '3' > Add quick comment doc'ing what id : 3 is Done, I also added an extra check about 'Fix Version' being set because I realized for JIRAs which don't have that field set, resolve action returns silently :). However, it's possible to add couple of lines to automatically set fixed release version field via cmdline args, what do you think ? PS2, Line 108: update_comment > Each of these functions constructs a new jira object (jira connection)...ca Yeah, that's kinda intentional, each of these actions eitehr can be independent or be chained after one another - in the latter case, next action should be bringing in a fresh object and not act upon stale object from previous action. I kinda faced this while testing one of the transitions, and hence localized the jira object to every routine itself. -- To view, visit http://gerrit.cloudera.org:8080/4852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4519ee0b83f9af03ba55f0eacc0553e86a3f13ec Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
