PR#265: standard-inventory-docker: port to python3 - standard-test-roles

standard-test-roles

#265 standard-inventory-docker: port to python3

Merged 5 years ago by astepano. Opened 5 years ago by ssahani.

ssahani/standard-test-roles python3-1 into master

standard-inventory-docker: port to python3

Susant Sahani • 5 years ago

6cc71cd

inventory/standard-inventory-docker

file modified

+4 -23

		`@@ -1,26 +1,6 @@`
		`- #!/usr/bin/python2`
		`-`
		`- # The MIT License (MIT)`
		`- #`
		`+ #!/usr/bin/python3`
		`+ # SPDX Licence identifier MIT`
		`# Copyright (c) 2017-2018 Red Hat Inc.`
		`- #`
		`- # Permission is hereby granted, free of charge, to any person obtaining a copy of`
		`- # this software and associated documentation files (the "Software"), to deal in`
		`- # the Software without restriction, including without limitation the rights to`
		`- # use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of`
		`- # the Software, and to permit persons to whom the Software is furnished to do so,`
		`- # subject to the following conditions:`
		`- #`
		`- # The above copyright notice and this permission notice shall be included in all`
		`- # copies or substantial portions of the Software.`
		`- #`
		`- # THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR`
		`- # IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS`
		`- # FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR`
		`- # COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER`
		`- # IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN`
		`- # CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.`
		`- #`
		`# Author: Merlin Mathesius <merlinm@redhat.com>`

		`import argparse`
		`@@ -173,7 +153,7 @@`
		`name = f.read().strip()`

		`# Need to figure out what python interpreter to use`
		`- interpreters = ["/usr/bin/python2", "/usr/bin/python3"]`
		`+ interpreters = ["/usr/bin/python3", "/usr/bin/python2"]`
		`for interpreter in interpreters:`
		`check_file = ["/usr/bin/docker", "exec", "--user=root", name, "/usr/bin/ls", interpreter]`
		`try:`
		`@@ -228,6 +208,7 @@`
		`if diagnose:`
		`def _signal_handler(*args):`
		`logger.info("Diagnose ending.")`
		`+`
		`logger.info("kill {0} # when finished".format(os.getpid()))`
		`signal.signal(signal.SIGTERM, _signal_handler)`
		`signal.pause()`

ssahani commented 5 years ago

Replace long licence text with SPDX identifier.
First look for python3.
_signal_handler: Add a line

astepano commented on line 35 of inventory/standard-inventory-docker 5 years ago

Hi, ssahani, could you please do not change any logic in re-factoring?
Would be you so kind to re-vert this change?

astepano commented on line 58 of inventory/standard-inventory-docker 5 years ago

Please re-vert, you made here endless recursion. _signal_handler calls self.

rebased onto 06b7cd20564d80015ae681701b71d3d256318521

5 years ago

ssahani commented 5 years ago

updated

astepano commented on line 35 of inventory/standard-inventory-docker 5 years ago

please re-vert =""

astepano commented on line 52 of inventory/standard-inventory-docker 5 years ago

please remove

ssahani commented on line 36 of inventory/standard-inventory-docker 5 years ago

Why ?

ssahani commented on line 53 of inventory/standard-inventory-docker 5 years ago

It's no readable at all . There should be a space ending a function.

astepano commented 5 years ago

Please send a new separate PR for code reformatting.
What this PR is about?
Fixing formatting or porting to Python3?
Please simplify review task for us.
Each PR - small logical change.

PS
Personally I do not like blank lines inside function. But, this is arguable personal preference.
https://www.python.org/dev/peps/pep-0008/#blank-lines
https://visualgit.readthedocs.io/en/latest/pages/code_style.html#blank-lines
https://softwareengineering.stackexchange.com/questions/251216/is-too-much-whitespace-a-bad-thing

ssahani commented 5 years ago

I don't agree with you. Is just simple stuff does not need any separate PR. What is so difficult just for spaces you need a separate PR

Edited 5 years ago by ssahani

ssahani commented 5 years ago

Again pasting a bunch of links from internet how it's helping?

astepano commented 5 years ago

@bgoncalv @mvadkert please review.

bgoncalv commented on line 36 of inventory/standard-inventory-docker 5 years ago

why was this change needed?

bgoncalv commented on line 53 of inventory/standard-inventory-docker 5 years ago

I think this blank line makes more clear that the function finished and the next commands are not part of it, but any decision you make here is fine by me.

bgoncalv commented 5 years ago

@ssahani could you just revert the change of get_artifact_path ? It has nothing to do with python3 porting. A part from it everything else looks good.

ssahani commented 5 years ago

@bgoncalv OK will do . Going to open a another PR for this

rebased onto b51efcc5d6c0f72423928ada9676cedff5672da3

5 years ago

ssahani commented 5 years ago

updated.

bgoncalv commented on line 49 of inventory/standard-inventory-docker 5 years ago

indentation changed, this should not be part of _signal_handler

rebased onto a12571886a3483a051470a8010c80810c2a134fd

5 years ago

ssahani commented 5 years ago

right updated thanks !

bgoncalv commented 5 years ago

lgtm

astepano commented on line 54 of inventory/standard-inventory-docker 5 years ago

@ssahani, could you please explain what was reason for this change?
https://docs.python.org/3/library/subprocess.html#subprocess.call
I see that Python3 does support : subprocess.call