Fix for k8s dict parsing #411
Conversation
|
Hi @vanpelt. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
Hi @vanpelt. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| "list": [1,2,3], | ||
| "time": datetime.datetime.now() | ||
| }) | ||
| assert isinstance(converted["time"], str) |
There was a problem hiding this comment.
Can we verify the whole converted results? Something like:
expected = {
"ENV": "test",
"number": 3,
"list": [1,2,3],
"time": 'YouNeedToUseAFixedTime'
}
self.assertEqual(expected, converted)
| @@ -141,6 +142,15 @@ def test_basic_workflow(self): | |||
| shutil.rmtree(tmpdir) | |||
| # print(tmpdir) | |||
|
|
|||
| def test_convert_k8s_obj_to_dic(self): | |||
| converted = compiler.Compiler()._convert_k8s_obj_to_dic({ | |||
There was a problem hiding this comment.
The argument given to _convert_k8s_obj_to_dic is not a k8s object. What exactly is this call trying to show?
| @@ -141,6 +142,15 @@ def test_basic_workflow(self): | |||
| shutil.rmtree(tmpdir) | |||
| # print(tmpdir) | |||
|
|
|||
| def test_convert_k8s_obj_to_dic(self): | |||
| converted = compiler.Compiler()._convert_k8s_obj_to_dic({ | |||
There was a problem hiding this comment.
_convert_k8s_obj_to_dic should not be an instance method. It's not using any compiler data.
|
How does this PR fix dict parsing? |
| @@ -508,7 +508,7 @@ def _convert_k8s_obj_to_dic(self, obj): | |||
| Returns: The serialized form of data. | |||
| """ | |||
|
|
|||
| from six import text_type, integer_types | |||
| from six import text_type, integer_types, iteritems | |||
There was a problem hiding this comment.
Is this a style change? This function is copied directly from the official Kubernetes client library.
There was a problem hiding this comment.
This is the bug. I hit this issue because I tried to pass a dict into op.add_env_variable. Line 541 accesses iteritems but it was only defined in a conditional branch so it threw an "object accessed before assignment" error.
There was a problem hiding this comment.
Another fix here may be to throw a ValueError if the env_variable isn't a k8s object, but It would be nice to be able to pass in a dict without having to use the k8s wrapper. Regardless, this is definitely a bug.
|
/ok-to-test |
|
Thank you @vanpelt! |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qimingj 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: qimingj 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 |
See kubeflow/testing#3523 If we are unable to find a common ancestor just compute the diff against the head of the master branch.
The current code blows up when attempting to parse a dict.
This change is