Pr15 split #16
Pr15 split #16
Conversation
Signed-off-by: Qin Zhang （张琴） <[email protected]> Signed-off-by: Andrew Cooper <[email protected]>
Signed-off-by: Qin Zhang （张琴） <[email protected]> Acked-by: Andrew Cooper <[email protected]>
andyhhp
requested review from
rosslagerwall,
MarkSymsCtx,
ashwin9390 and
bernhardkaindl
There was a problem hiding this comment.
@andyhhp, great PR!
It is definitely a big step into the right direction, that is much preferred!
I'd love this approach!
- Simple and 100% obvious Python2.7-safe fixes first!
- Keep compatibility with Python 2.7!
The reason I support this wholeheartedly is that while readying a whole slew of fixes of https://github.com/xenserver/status-report for XS 8.2 CU1 (which has no useful Python3, so it's Python2.7 only), both @edwintorok and @rosslagerwall asked for the backport to be done by not branching the code and developing individual backports for each fix but use the current upstream version instead, saving us from doubling the maintenance work for two branches.
This is only possible because https://github.com/xenserver/status-report still supports Python2.7, so we do not have to fork the code.
While I've done a port of bugtool to Python3.6 xenserver/status-report@master...t_bernhardk/2to3-10-2to3-fix-filter+1 it became clear that testing intrusive changes and correct function is a nightmare:
All outputs of bugtool have to be checked for regressions, and for some, you need the data present.
For example, you need XenServer pool(s) with all possible storage types so that you get to test that the data for all storage (and other various XenServer configurations) is properly collected.
Even without such extended testing conducted so far, my Python3 branches grew quickly to 37 commits, far too much to be reviewed in a single pull request!
Thankfully, I also started out like this PR (but without the global removal of terminating space), so it is easy to split the obviously 100% correct commits from those which need to be checked more closely!
Like xsconsole, bugtool had no sort of automated unit or integration testing.
For bugtool, I then was asked instead to merge other unrelated improvements (like human-readable load reports from sar). Immediately, I wrote tests with fake data for this, so now there is some minimal GitHub CI, but it's far from a good test coverage.
I also gave running GitHub CI on an actual XenServer Dom0 a spin:
I ran an external Dom0-GitHub runner on actual real XenServer Dom0.
My work exploring this is in a separate repo of mine: https://github.com/xenserver-next/status-report/tree/dom0-tests.
But back to xsconsole:
Update: I had a reservation on the commit removing the whitespace because of #4 and #14 then having to be rebased with git rebase --ignore-whitespace, but I agree that we should not hold this up, as Andy commented below - approval from me.
…usoutput() Fix the 1st commit of #17 which replaced commands.getstatusoutput() with subprocess.getstatusoutput() without taking the change of the status code into account: Unfortunately, the Python3 developers broke the compatibilty: The return code changes: python2 -c 'import commands ; print( commands.getstatusoutput("false"))' (256, '') python3 -c 'import subprocess; print(subprocess.getstatusoutput("false"))' (1, '') With commands.getstatusoutput(), you had to use this to get the actual exit code: status = os.WEXITSTATUS(status) These calls have to be removed because now they just shift away the error code: As shown at benjaminp/six#207, the operation is just `status >> 8` Lucily, the status code is checked to against 0 at most places, so there is no change for these checks. There is only one location where a bit is checked. Fix this location too. Also, that commit did not take into account that subprocess.get*output do not exist in Python2, which goes against the directive by Andrew in PR #16 where he requires that we keep Python2 working: The current master branch works neither for Python2, nor Python3 - fix this breakage in the 2nd commit. Signed-off-by: Bernhard Kaindl <[email protected]>
…usoutput() Fix the 1st commit of #17 which replaced commands.getstatusoutput() with subprocess.getstatusoutput() without taking the change of the status code into account: Unfortunately, the Python3 developers broke the compatibility: The return code changes: python2 -c 'import commands ; print( commands.getstatusoutput("false"))' (256, '') python3 -c 'import subprocess; print(subprocess.getstatusoutput("false"))' (1, '') With commands.getstatusoutput(), you had to use this to get the actual exit code: status = os.WEXITSTATUS(status) These calls have to be removed because now they just shift away the error code: As shown at benjaminp/six#207, the operation is just `status >> 8` Luckily, the status code is checked to against 0 at most places, so there is no change for these checks. There is only one location where a bit is checked. Fix this location too. Also, that commit did not take into account that subprocess.get*output do not exist in Python2, which goes against the directive by Andrew in PR #16 where he requires that we keep Python2 working: The current master branch works neither for Python2, nor Python3 - fix this breakage in the 2nd commit. Signed-off-by: Bernhard Kaindl <[email protected]>
…usoutput() Fix the 1st commit of #17 which replaced commands.getstatusoutput() with subprocess.getstatusoutput() without taking the change of the status code into account: Unfortunately, the Python3 developers broke the compatibility: The return code changes: python2 -c 'import commands ; print( commands.getstatusoutput("false"))' (256, '') python3 -c 'import subprocess; print(subprocess.getstatusoutput("false"))' (1, '') With commands.getstatusoutput(), you had to use this to get the actual exit code: status = os.WEXITSTATUS(status) These calls have to be removed because now they just shift away the error code: As shown at benjaminp/six#207, the operation is just `status >> 8` Luckily, the status code is checked to against 0 at most places, so there is no change for these checks. There is only one location where a bit is checked. Fix this location too. Also, that commit did not take into account that subprocess.get*output do not exist in Python2, which goes against the directive by Andrew in PR #16 where he requires that we keep Python2 working: The current master branch works neither for Python2, nor Python3 - fix this breakage in the 2nd commit. Signed-off-by: Bernhard Kaindl <[email protected]>
Split some trivial bits out of #15
This is literally just removing trailing whitespace, and switching to "except blah as" syntax.