Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python docker image build #141

Closed
wants to merge 11 commits into from

Conversation

rienafairefr
Copy link
Contributor

The docker building was limited to the JS/Typescript runtime, and it's been missing when using the Python runtime. @adir-intsights issue #129 mentioned it. I went ahead and straight ported the javascript code. There is now a py example dockerfile-py which builds and tag an image, it seems to work.

There is still an issue with async code I can't pinpoint (something in the Output/apply does not await), /usr/lib/python3.7/asyncio/base_events.py:405: RuntimeWarning: coroutine 'Output.future.<locals>.get_value' was never awaited, but at least the image is built fine. The rest of the functionalities (connect to registry, push, etc), are untested, and may be badly ported so care/test is needed. Python versions compatilibites might be fucked up as well, tested only on py 3.7

Fixes: #129

@stack72
Copy link
Contributor

stack72 commented Feb 26, 2020

OMG! This is fantastic @rienafairefr

I am testing this right now :)

@stack72
Copy link
Contributor

stack72 commented Feb 26, 2020

Hi @rienafairefr

Ok, I have figured out the issue with the build on the init.py file and opened the bug linked for this. There is 1 small issue in your PR - the new test doesn't have a requirements.txt file in it so it's failing to build the deps

I will be able to shepherd this across the line once up have the upstream bug fixed

Thanks so much for all your work here

Paul

@stack72 stack72 self-requested a review February 26, 2020 12:00
@stack72 stack72 self-assigned this Feb 26, 2020
@rienafairefr
Copy link
Contributor Author

That's a weird edge case pulumi/pulumi-terraform-bridge#119 that this PR uncovered, well, let's wait for that. I've added what I think was the missing requirements.txt :-)

@stack72
Copy link
Contributor

stack72 commented Feb 29, 2020

@rienafairefr I will be working on fixing that up next week :) I am very keen to get this work into the system!

stack72 added a commit to pulumi/pulumi that referenced this pull request Mar 9, 2020
Fixes: pulumi/pulumi-terraform-bridge#119

This allows us to specify an overlays block e.g.

```
Overlay: &tfbridge.OverlayInfo{
	DestFiles: []string{
		"pulumi_docker/docker.py",
		"pulumi_docker/image.py",
	},
},
```

The overlays files are treated differently to normal module files
as they are not generated. This structure means that we will emit
the correct entries in the __init__.py file

Without this structure (ie. pulumi_pkgname), the generator actually
copies the file (i.e. docker.py) to the root of the Python SDK. This
is because the structure of the Python SDK has a sub-folder than that
of the NodeJS SDK

I tested this using PR pulumi/pulumi-docker#141
and this now works as expected and we can take advantage of the new
Python overlays for Docker
stack72 added a commit to pulumi/pulumi that referenced this pull request Mar 9, 2020
Fixes: pulumi/pulumi-terraform-bridge#119

This allows us to specify an overlays block e.g.

```
Overlay: &tfbridge.OverlayInfo{
	DestFiles: []string{
		"pulumi_docker/docker.py",
		"pulumi_docker/image.py",
	},
},
```

The overlays files are treated differently to normal module files
as they are not generated. This structure means that we will emit
the correct entries in the __init__.py file

Without this structure (ie. pulumi_pkgname), the generator actually
copies the file (i.e. docker.py) to the root of the Python SDK. This
is because the structure of the Python SDK has a sub-folder than that
of the NodeJS SDK

I tested this using PR pulumi/pulumi-docker#141
and this now works as expected and we can take advantage of the new
Python overlays for Docker
stack72 added a commit to pulumi/pulumi that referenced this pull request Mar 9, 2020
Fixes: pulumi/pulumi-terraform-bridge#119

This allows us to specify an overlays block e.g.

```
Overlay: &tfbridge.OverlayInfo{
	DestFiles: []string{
		"pulumi_docker/docker.py",
		"pulumi_docker/image.py",
	},
},
```

The overlays files are treated differently to normal module files
as they are not generated. This structure means that we will emit
the correct entries in the __init__.py file

Without this structure (ie. pulumi_pkgname), the generator actually
copies the file (i.e. docker.py) to the root of the Python SDK. This
is because the structure of the Python SDK has a sub-folder than that
of the NodeJS SDK

I tested this using PR pulumi/pulumi-docker#141
and this now works as expected and we can take advantage of the new
Python overlays for Docker
@rienafairefr
Copy link
Contributor Author

Now that pulumi/pulumi-terraform-bridge#119 is down, what's the next step ?

@stack72
Copy link
Contributor

stack72 commented Mar 24, 2020

@rienafairefr apologies - I was away for a little while - I am working on integrating this to master now :)

@stack72
Copy link
Contributor

stack72 commented Mar 24, 2020

Hi @rienafairefr

Unfortunately, I am not able to get this to work... I made a few small changes to the PR as follows:

diff --git a/Makefile b/Makefile
index 00d336b..c4b5377 100644
--- a/Makefile
+++ b/Makefile
@@ -69,6 +69,8 @@ install::
                yarn install --offline --production && \
                (yarn unlink > /dev/null 2>&1 || true) && \
                yarn link
+       cd ${PACKDIR}/python/bin && $(PIP) install --user -e .
+       echo "Copying NuGet packages to ${PULUMI_NUGET}"
        [ ! -e "$(PULUMI_NUGET)" ] || rm -rf "$(PULUMI_NUGET)/*"
        find . -name '$(NUGET_PKG_NAME).*.nupkg' -exec cp -p {} ${PULUMI_NUGET} \;

@@ -77,10 +79,10 @@ istanbul_tests::
        cd sdk/nodejs/tests && yarn run mocha $$(find bin -name '*.spec.js')

 test_fast:: istanbul_tests
-       $(GO_TEST_FAST) ./examples/...
+       $(GO_TEST_FAST) ./examples/

 test_all:: istanbul_tests
-       $(GO_TEST) ./examples/...
+       $(GO_TEST) ./examples/
--- a/build/common.mk
+++ b/build/common.mk
@@ -105,8 +105,8 @@ PULUMI_BIN          := $(PULUMI_ROOT)/bin
 PULUMI_NODE_MODULES := $(PULUMI_ROOT)/node_modules
 PULUMI_NUGET        := $(PULUMI_ROOT)/nuget

-GO_TEST_FAST = go test -short -v -count=1 -cover -timeout 2h -parallel ${TESTPARALLELISM}
-GO_TEST = go test -v -count=1 -cover -timeout 2h -parallel ${TESTPARALLELISM}
+GO_TEST_FAST = go test -v -count=1 -timeout 2h -parallel ${TESTPARALLELISM}
+GO_TEST = go test -v -count=1 -timeout 2h -parallel ${TESTPARALLELISM}
diff --git a/resources.go b/resources.go
index 7918169..e3ee2c0 100644
--- a/resources.go
+++ b/resources.go
@@ -145,6 +145,12 @@ func Provider() tfbridge.ProviderInfo {
                        Requires: map[string]string{
                                "pulumi": ">=1.0.0,<2.0.0",
                        },
+                       Overlay: &tfbridge.OverlayInfo{
+                               DestFiles: []string{
+                                       "pulumi_docker/docker.py",
+                                       "pulumi_docker/image.py",
+                               },
+                       },
                },

These changes allow this to build as expected.

but when I run the tests, dockerfile-py fails with the following error:

[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 187, in do_build
[ -docker/examples/dockerfile-py ]         image_name, path_or_build_val, repository_url_val, log_resource, connect_to_registry, skip_push)
[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 278, in build_and_push_image_worker_async
[ -docker/examples/dockerfile-py ]         build_result = await build_image_async(base_image_name, path_or_build, log_resource, cache_from)
[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 412, in build_image_async
[ -docker/examples/dockerfile-py ]         await docker_build(image_name, build, log_resource, cache_from)
[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 467, in docker_build
[ -docker/examples/dockerfile-py ]         return await run_command_that_must_succeed("docker", build_args, log_resource, env=build.env)
[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 588, in run_command_that_must_succeed
[ -docker/examples/dockerfile-py ]         cmd, args, log_resource, report_full_command_line, False, stdin, env)
[ -docker/examples/dockerfile-py ]       File "/Users/stack72/code/go/src/github.com/pulumi/pulumi-docker/sdk/python/bin/pulumi_docker/docker.py", line 650, in run_command_that_can_fail
[ -docker/examples/dockerfile-py ]         process = subprocess.Popen(popen, env=env, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
[ -docker/examples/dockerfile-py ]       File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 800, in __init__
[ -docker/examples/dockerfile-py ]         restore_signals, start_new_session)
[ -docker/examples/dockerfile-py ]       File "/usr/local/Cellar/python/3.7.7/Frameworks/Python.framework/Versions/3.7/lib/python3.7/subprocess.py", line 1551, in _execute_child
[ -docker/examples/dockerfile-py ]         raise child_exception_type(errno_num, err_msg, err_filename)
[ -docker/examples/dockerfile-py ]     FileNotFoundError: [Errno 2] No such file or directory: 'docker': 'docker'
[ -docker/examples/dockerfile-py ]     error: an unhandled error occurred: Program exited with non-zero exit code: 1

I 100% have docker available because it's running all of the other tests. It seems t be failing when trying to check the docker build . Thoughts here? It seems to not be able to launch the docker subprocess which seems strange

Paul

@rienafairefr
Copy link
Contributor Author

Thanks Paul @stack72 for the review.
I incorporated your patches on the overlay & makefiles,
On my ubuntu machine the build works, I suspect you're using a mac os because of the /Users
Just verified, the subprocess.Popen call sends the correct ['docker', 'build' ...], I feared I sent something like ["'docker'", 'build' ...] (quotes inside quotes), but it's apparently not the case. The Python script runs under a runtime under the control of the pulumi process, maybe there is something fishy in the PATH of that process, in the case of mac os, and it can't find the docker executable.

Looking at it again, the way the env is sent into the subprocess might be why it's failing. And there have been differences between Mac OS/other OSs with regard to that subprocess.Popen call when env is concerned.
Maybe in my OS, with env=None, the subprocess sees the same env as the main process (including PATH), while in your case, with env=None, the environnement of the subprocess is completely empty with env=None, something like that.

I'll see that the env= value passed to docker build augments the existing env instead of overwriting, that might fix the issue

example mac os-specific issue with subprocess env: https://bugs.python.org/issue12383

@rienafairefr
Copy link
Contributor Author

Yep, as suspected, from the python docs, "If env is not None, it must be a mapping that defines the environment variables for the new process; these are used instead of the default behavior of inheriting the current process’ environment. It is passed directly to Popen."
It's probably only chance that passing an empty dict {} works in my case

@rienafairefr
Copy link
Contributor Author

@stack72 I pushed a fix for the env thing, now the docker... subprocess gets the same environment as the calling process, plus update for env variables that one might want to add (.e.g DOCKER_BUILDKIT=1)

@stack72
Copy link
Contributor

stack72 commented Mar 25, 2020

Ah @rienafairefr

This is a great fix - I just ran the tests right now and everything is working as expected :) I cannot thank you enough for this work! It's going to make life a lot easier for everyone using this package

I am going to merge this manually as I have 1 formatting fix to the Makefile to update - and I will rebase this

Thanks again!!

Paul

@stack72 stack72 closed this Mar 25, 2020
@stack72
Copy link
Contributor

stack72 commented Mar 25, 2020

Manually merged and pushed to master!

@rienafairefr
Copy link
Contributor Author

Good news @stack72 ! There's still an issue with the async code that I mentioned (/usr/lib/python3.7/asyncio/base_events.py:405: RuntimeWarning: coroutine 'Output.future.<locals>.get_value' was never awaited), but I'll file a separate issue to get to the bottom of this 👍

@rienafairefr rienafairefr deleted the py-docker-image branch March 26, 2020 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants