enable component build unit test #228
Conversation
| @@ -86,22 +84,17 @@ def _wrap_files_in_tarball(self, tarball_path, files={}): | |||
| for key, value in files.items(): | |||
| tarball.add(value, arcname=key) | |||
|
|
|||
| def prepare_docker_tarball_with_py(self, arc_python_filename, python_filepath, base_image): | |||
| def prepare_docker_tarball_with_py(self, arc_python_filename, python_filepath, base_image, local_tarball_path): | |||
There was a problem hiding this comment.
How about prepare_docker_tarball_with_py(self, python_filepath, local_tarball_path, base_image='python:3.5', arc_python_filename='program.py')
BTW, can python_filepath be absolute?
There was a problem hiding this comment.
Good question, the python_filepath can be absolute or relative.
I think it might be better not to give default values, or else the error messages might be misleading once the container launched but the program.py is not found.
There was a problem hiding this comment.
Good question, the python_filepath can be absolute or relative.
That's good. The problem is that your code will fail in one of those cases.
There was a problem hiding this comment.
the error messages might be misleading once the container launched but the program.py is not found.
How come this file is not in the container? You're putting it there yourself.
There was a problem hiding this comment.
You are right. I'm good with default value for arc_python_filename. However, I do not think we should add a default value for base_image. This parameter should be mandatory from the DockerfileHelper if not for out layer APIs.
|
I'm approving this PR, but these code files have bugs and also need some refactoring. We should address it in new PRs. |
|
/lgtm |
Do you mind pointing out where the bugs are? Are the bugs you mentioned related to the new codes in this PR or those already exist before this PR? Thanks. If there are bugs that are introduced by this PR, I will not hesitate to address them in this PR instead of putting off to future PRs. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaoning777 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaoning777 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…#228) * Fix for ks 0.12.0 * Fix test and lint * Fix pylint * Check apiVersion in app.yaml * Fix test * Fix test
* avoid double mutation of deployments * Revert "avoid double mutation of deployments" This reverts commit ceb8b209628fdb8e013ac7ee900b51e34881bce4. * model initializer injector should be idempotent
This change is