PR#270: Port to python3: inventory-qcow2, inventory-rpm and inventory-local - standard-test-roles

standard-test-roles

#270 Port to python3: inventory-qcow2, inventory-rpm and inventory-local

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

ssahani/standard-test-roles port-python3-2 into master

standard-inventory-rpm: port to python3

Susant Sahani • 5 years ago

50c9f70

standard-inventory-qcow2: port to python3

Susant Sahani • 5 years ago

a90e459

standard-inventory-local: port to python3

Susant Sahani • 5 years ago

4b7ced6

inventory/standard-inventory-local

file modified

+2 -22

		`@@ -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: Stef Walter <stefw@redhat.com>`

		`import argparse`

inventory/standard-inventory-qcow2

file modified

+2 -2

		`@@ -1,4 +1,4 @@`
		`- #!/usr/bin/python2`
		`+ #!/usr/bin/python3`

		`import argparse`
		`import atexit`
		`@@ -443,7 +443,7 @@`
		`child = os.fork()`
		`if child:`
		`# Need to figure out what python interpreter to use`
		`- interpreters = ["/usr/bin/python2", "/usr/bin/python3", "/usr/libexec/platform-python"]`
		`+ interpreters = ["/usr/bin/python3", "/usr/bin/python2", "/usr/libexec/platform-python"]`
		`for interpreter in interpreters:`
		`check_file = [`
		`"/usr/bin/ansible",`

inventory/standard-inventory-rpm

file modified

+2 -22

		`@@ -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.`
		`- #`
		`# Authors: Merlin Mathesius <merlinm@redhat.com>`
		`# Stef Walter <stefw@redhat.com>`

ssahani commented 5 years ago

None

astepano commented on line 34 of inventory/standard-inventory-local 5 years ago

@ssahani, could you explain this change please?

This function in original python 2 script has an optional parameter: path
This function in python 3 script has one required positional parameter: path

You completely change function specification.

Before function could be called in 3 different ways:

get_artifact_path()
get_artifact_path(path="/path")
get_artifact_path("/path")

With this change you forbid to use methods 1 and 2.

I asked you to revert your changes for the same place at https://pagure.io/standard-test-roles/pull-request/265#_1,34

Edited 5 years ago by astepano

astepano commented 5 years ago

Please check : commits history: https://pagure.io/standard-test-roles/commits/master

If I marge your PR what commit comment will look like? It will be:

Merge 270 Port to python3: another set

Please change this comment to appropriate meaningful form, that will help read git history.

ssahani commented 5 years ago

Before function could be called in 3 different ways:

get_artifact_path()
get_artifact_path(path="/path")
get_artifact_path("/path")

Let's see the usage of the function how this is used.

'log_file = get_artifact_path(LOG_FILE) '
Now where you plan to use it with a black value.
option 2 and 3 still works.

ssahani commented 5 years ago

Please check : commits history: https://pagure.io/standard-test-roles/commits/master
If I marge your PR what commit comment will look like? It will be:
Merge 270 Port to python3: another set
Please change this comment to appropriate meaningful form, that will help read git history.

you sure Note: it contains 3 PRS but yes should be properly should be named

Edited 5 years ago by ssahani

astepano commented 5 years ago

@ssahani, you have a very clear task: "port scripts to Python 3". Please go and read your Jira ticket.
In this PR you have changed function specification. This is unrelated to Python3 porting.
In your PR comment you say: "This is about python3 porting". But, it is not.
I am trying to review your PR according to your PR comment. Please explain, how this change to function specification is related to Python3 porting?

@bgoncalv @mvadkert @bookwar please review this RP.

astepano commented 5 years ago

@ssahani , please write comments to your PR in readable format.

you sure Note: it contains 3 PRS but yes should be properly should be named

Above comment very hard to read and understand. Please add punctuation marks. Is this statement or question? Please save our time.

ssahani commented 5 years ago

@ssahani, you have a very clear task: "port scripts to Python 3". Please go and read your Jira ticket.

Please don't try to make me understand what is my task.

In this PR you have changed function specification. This is unrelated to Python3 porting.
In your PR comment you say: "This is about python3 porting". But, it is not.

your function specification does not have any use case.

ssahani commented 5 years ago

If I marge your PR what commit comment will look like? It will be:
it's not marge it's merge. Please fix your spelling first

astepano commented 5 years ago

If I marge your PR what commit comment will look like? It will be:
it's not marge it's merge. Please fix your spelling first

You are right, sorry.

astepano 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.

rebased onto 4b7ced6

5 years ago

ssahani commented 5 years ago

updated