Check is not None in safe_get #158
Conversation
|
Welcome @qlemaire22! |
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
I signed it! |
k8s-ci-robot
added
cncf-cla: yes
qlemaire22
changed the title
qlemaire22
changed the title
|
/assign @roycaihw |
config/kube_config.py
Outdated
| @@ -494,7 +494,7 @@ def __len__(self): | |||
|
|
|||
| def safe_get(self, key): | |||
| if (isinstance(self.value, list) and isinstance(key, int) or | |||
| key in self.value): | |||
| self.value is not None and key in self.value): | |||
There was a problem hiding this comment.
Please add a test for this. Do you have a kubeconfig yaml that reproduces the error?
self.value should be either a list or a dict. I think we should restrict ConfigNade with None value early if we have such a case.
There was a problem hiding this comment.
You are right. In my case, it happened when calling _get_kube_config_loader_for_yaml_file with no kubeconfig available. Should I raise an error somewhere like in KubeConfigMerger, KubeConfigLoader or ConfigNode to warn about empty kube_config?
There was a problem hiding this comment.
could you add checks in both _get_kube_config_loader_for_yaml_file and KubeConfigLoader?
In _get_kube_config_loader_for_yaml_file an error should be raised if kcfg.config is None. This is likely be caused by invalid kubeconfig file path. This error restores the old behavior we had
In KubeConfigLoader an error should be raised if the input config_dict is None. This makes the error more obvious if someone (unlikely) uses the KubeConfigLoader directly with a None config_dict object
There was a problem hiding this comment.
I did add the two checks. Please, tell me if I should put more specific error messages.
k8s-ci-robot
added
size/S
Codecov Report
@@ Coverage Diff @@
## master #158 +/- ##
==========================================
- Coverage 93.56% 93.44% -0.13%
==========================================
Files 13 13
Lines 1384 1388 +4
==========================================
+ Hits 1295 1297 +2
- Misses 89 91 +2
Continue to review full report at Codecov.
|
config/kube_config.py
Outdated
| if isinstance(config_dict, ConfigNode): | ||
| if config_dict is None: | ||
| raise ConfigException( | ||
| 'Invalid kube-config file. ' |
There was a problem hiding this comment.
config_dict here doesn't necessarily come from a kubeconfig file. Change the error message to Invalid kube config. Expected config_dict to not be None.
There was a problem hiding this comment.
Right, I changed the error message.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qlemaire22, roycaihw 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 |
k8s-ci-robot
added
the
approved
Hi,
I would add a check to see if
self.valueis not None insafe_get.Otherwise it can create a
TypeError: argument of type 'NoneType' is not iterablewhich I faced when working on Apache Airflow. I believe that I would be easier to debug.