Recode extract_image_id.sh in python (as well as the compare_ids_test rule) #448

ArthurRab · 2018-07-05T20:57:31Z

Changed extract_image_id.sh to extract_image_id.py,
all uses of it will need to be converted over.

nlopezgi · 2018-07-06T13:27:11Z

contrib/compare_ids_test.sh.tpl


 for image in {tars}
 do
-  current_id=$({id_script_path} $image)


I apologize, I thought this pattern of using sh.tpl + python was common, but I looked in more detail and this is the wrong approach to invoke a python tool from a skylark rule.
I looked at examples, what you need to do is:

Make this also a python script

create a build target for the python script (one for each if you expect users to use extract_image_id.py independently of this test) (e.g., this is the target for extract_config.py: https://github.com/bazelbuild/rules_docker/blob/master/container/BUILD#L28)

In the skylark rule declare a dependency on the python target (e.g., this is how layer_tools declares a dep on a script to extract a config: https://github.com/bazelbuild/rules_docker/blob/master/container/layer_tools.bzl#L196)

Use the python target as executable from the skylark rule (e.g., this is how the extract config script is invoked: https://github.com/bazelbuild/rules_docker/blob/master/container/layer_tools.bzl#L23)

Also related, how similar is the functionality you have in your py script to the one in extract_config.py? could you reuse extract_config.py (asking as that script claims it extracts the v2.2 config file)?

Discussed offline. Used similar but modified method to make it work for a test.

nlopezgi · 2018-07-18T18:14:20Z

contrib/compare_ids_test.bzl

            "{id}": ctx.attr.id,
-            "{tars}": tars_string,
-            "{id_script_path}": ctx.file._id_extract_script.short_path,
+            "{tars}": repr([i.short_path for i in tar_files]),


These can be passed as args to a pythons script, no need to use a template.
You can use ctx.actions.run (e.g., https://github.com/bazelbuild/rules_docker/blob/master/contrib/passwd.bzl#L80) using as executable the py_binary target (note how the example uses the //container:build_tar target as the executable)

nlopezgi · 2018-07-18T18:16:09Z

contrib/BUILD

 exports_files(["structure-test.sh.tpl"])

-exports_files(["compare_ids_test.sh.tpl"])
+exports_files(["compare_ids_test.py.tpl"])


This should be a python script, not a template

this build file should not have an exports_files target, it should have a py_binary target(s) (e.g., https://github.com/bazelbuild/rules_docker/blob/master/container/BUILD#L67)

nlopezgi · 2018-07-18T18:23:15Z

contrib/compare_ids_test.bzl

            allow_files = True,
            single_file = True,
-            default = "compare_ids_test.sh.tpl",
+            default = "compare_ids_test.py.tpl",


see https://github.com/bazelbuild/rules_docker/blob/master/contrib/passwd.bzl#L137

…st way to do it

Also updated compare_ods_fail_test to work for this as well

nlopezgi · 2018-07-19T13:59:52Z

contrib/BUILD

 exports_files(["compare_ids_test.bzl"])

-exports_files(["extract_image_id.sh"])
+exports_files(["extract_image_id.py"])


this should not need to export this file?

I use this file in the compare_ids_fail_test.bzl as it needs to copy it into the new workspace it produces to test the failing cases of the compare_ids_test rule.

nlopezgi · 2018-07-19T14:14:06Z

contrib/compare_ids_test.bzl

+    # The compare_ids_test target (produced by the py_binary rule) produces 2 files,
+    # the executable and the source code. This retrieves the executable. A better method
+    # of retrieving it would be ideal, but I could not find one.
+    compare_ids_test_exectuable = ctx.files._compare_ids_test_script[1]


use ctx.executables https://docs.bazel.build/versions/master/skylark/lib/ctx.html#executable

nlopezgi · 2018-07-19T14:30:33Z

tests/contrib/compare_ids_fail_test.bzl


 def _impl(ctx):
    test_code = """ '
+py_binary(


why are you need this duplicated from the BUILD file above here?
If its because your test is rewriting the file contrib/BUILD, then do not re write it, write a new BUILD file in some subdirectory (e.g., contrib/fail_test/BUILD)

Changed it to copy the BUILD instead of duplicate code

Made the compare_ids_fail_test copy the contrib/BUILD file so as to not have to duplicate code

nlopezgi

this looks good overall, could you also ask Xin to go over the python bits and review

xingao267 · 2018-07-19T20:57:30Z

contrib/compare_ids_test.py

 # See the License for the specific language governing permissions and
 # limitations under the License.
+import sys
+import os


os and sys are not used. Can you run lint on all the py files

Yup. Did pyformat, forgot lint. Fixed.

xingao267 · 2018-07-19T21:30:22Z

contrib/extract_image_id.py


 if __name__ == "__main__":
-  tar_path = sys.argv[1]
+  path = sys.argv[1]


no need to create variable path

googlebot added the cla: yes label Jul 5, 2018

ArthurRab requested review from nlopezgi and xingao267 July 5, 2018 20:58

ash2k mentioned this pull request Jul 5, 2018

Image digest output for container_image rule #445

Merged

nlopezgi reviewed Jul 6, 2018

View reviewed changes

nlopezgi reviewed Jul 18, 2018

View reviewed changes

Arthur Rabinovich added 15 commits July 18, 2018 16:30

Added python version of extract_image_id.sh

831991e

Changed compare_ids_test to use the python script

c36f6b4

Add invalid tar message to python script

1efa388

Changed it to work with python2

f02bb10

Updated the failing test rule to use the .py script

45a7608

Removed the old sh script

f2bab73

Comments

1bcf506

Formatting

6b0b88a

Removed export of deleted file

847749d

Comment correction

8fcfae7

Changed the test to use a .py.tpl as well. Not sure if this is the be…

a463bd2

…st way to do it

WIP

c093c40

Added py_binary for extract_image_id

ccc3cca

Changed compare_ids_test to python

932530c

Also updated compare_ods_fail_test to work for this as well

Changed str concat to .format

d359757

ArthurRab changed the title ~~Recode extract_image_id.sh in python~~ Recode extract_image_id.sh in python (as well as the compare_ids_test rule) Jul 18, 2018

nlopezgi reviewed Jul 19, 2018

View reviewed changes

ArthurRab and others added 3 commits July 19, 2018 11:24

Reduce code duplication

532e941

Made the compare_ids_fail_test copy the contrib/BUILD file so as to not have to duplicate code

Merge branch 'master' into recode_extract_image_id.sh_in_python

59d3858

buildifier

fbb6918

nlopezgi approved these changes Jul 19, 2018

View reviewed changes

Formatting

028fec9

xingao267 reviewed Jul 19, 2018

View reviewed changes

pylint formatting fixes

4a8e6e1

xingao267 reviewed Jul 19, 2018

View reviewed changes

Removed unnecessary variable

69f7c62

xingao267 merged commit a25f00c into bazelbuild:master Jul 20, 2018

ArthurRab deleted the recode_extract_image_id.sh_in_python branch July 20, 2018 15:00

Recode extract_image_id.sh in python (as well as the compare_ids_test rule) #448

Recode extract_image_id.sh in python (as well as the compare_ids_test rule) #448

Uh oh!

Conversation

ArthurRab commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlopezgi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants