Extend YAML load functionality to *LIST and multi-resources #673
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/cc @roycaihw |
|
In response to this:
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. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
micw523
force-pushed
the
yaml-list
branch
3 times, most recently
from
b6b93ec
to
d52c7ac
Compare
|
/hold cancel |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
kubernetes/utils/create_from_yaml.py
Outdated
| if output_list is False: | ||
| if len(k8s_api_all) == 1: | ||
| return k8s_api_all[0] | ||
| return k8s_api_all |
There was a problem hiding this comment.
Have you verified kubectl's behavior? What if we failed creating some of the items?
There was a problem hiding this comment.
I think I only tested on 404 with an invalid API version and 409 with conflicts. These exceptions are handled by API internally.
I probably should go test with a multi resource yaml with one of the resources being 404/409. I’ll keep you updated.
|
The base module update is an overlook. I should fix that... |
k8s-ci-robot
added
approved
|
/open |
k8s-ci-robot
added
size/L
micw523
force-pushed
the
yaml-list
branch
2 times, most recently
from
2018925
to
dd62474
Compare
|
/meow psychedelic |
|
@fejta: Bad category. Please see https://api.thecatapi.com/api/categories/list In response to this:
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. |
Dunno... We just send it to thecatapi, not sure why it's including it in the list but not returning a valid image. https://api.thecatapi.com/api/images/get?format=json&results_per_page=1&category=psychedelic returns an empty list is the problem |
|
@fejta I still want my code reviewed here😂 |
|
@micw523: Bad category. Please see https://api.thecatapi.com/api/categories/list In response to this:
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. |
|
/meow sunglasses |
|
In response to this:
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. |
| :param async_req bool | ||
| :param bool include_uninitialized: If true, partially initialized resources are included in the response. | ||
| :param str pretty: If 'true', then the output is pretty printed. | ||
| :param str dry_run: When present, indicates that modifications should not be persisted. An invalid or unrecognized dryRun directive will result in an error response and no further processing of the request. Valid values are: - All: all dry run stages will be processed | ||
| """ | ||
|
|
||
| k8s_api_all = [] | ||
| with open(path.abspath(yaml_file)) as f: |
There was a problem hiding this comment.
path.abspath(yaml_file)
will crash on string input while yaml.load(str) can load the yaml from a string.
very useful if you need to change the yaml dynamically
ill suggest to support yaml_file as string and a file
There was a problem hiding this comment.
I don’t think this is necessary. The feature requested is to replicate kubectl create -f which of course would not support strings. YAML files can be load as a dictionary and passed as the body parameter when the user calls the corresponding APIs. I don’t see the benefit of changing a YAML on the fly - we’re not doing kubectl apply -f here.
If you want this feature to parse as a string, please file a separate issue.
There was a problem hiding this comment.
I tried running the example but hit following error
$ python examples/create_deployment_from_yaml.py
Traceback (most recent call last):
File "examples/create_deployment_from_yaml.py", line 17, in <module>
from kubernetes import client, config, utils
ImportError: cannot import name utils
did I miss something?
| try: | ||
| k8s_api_all.append(create_from_yaml_single_item( | ||
| k8s_client, yml_document, verbose, **kwargs)) | ||
| except client.rest.ApiException as api_exception: |
There was a problem hiding this comment.
it is great that the util runs through the entire list even when there is a failure, like kubectl does
however kubectl returns error code 1 to stderr when failure happens, I'd expect we raise error to upstream after we finish the entire list, to make it easier for automation
There was a problem hiding this comment.
instead of print each failure. I'd like to see we concatenate the errors and raise in the end
for question about what the raised error should look like, as long as the error clearly identifies failed objects and is well-formatted to parse (for upstream automation) when necessary, I'm okay with having either a string or a new class for the error message. @mbohlool @yliaog Do you have suggestion on what's the best practice?
below is what kubectl does
$ kubectl create -f hack/testdata/list.yaml 1>/dev/null
Error from server (AlreadyExists): services "list-service-test" already exists
Error from server (AlreadyExists): deployments.apps "list-deployment-test" already exists
kubernetes/e2e_test/test_utils.py
Outdated
| namespace="default", body={}) | ||
|
|
||
| def test_multi_resource_with_conflict(self): |
There was a problem hiding this comment.
need to document the tests. it's hard to follow what's expected from reading the test and file names
k8s-ci-robot
added
size/XL
| Using core, extensions API | ||
| Using yaml file core-namespace-dep.yaml | ||
| Using yaml file extensions-deployment-dep.yaml | ||
| """ |
There was a problem hiding this comment.
def test_create_deployment_in_namespace_from_yaml(self):
"""
Should be able to create a deployment in a non-default namespace.
"""
| Check the program raises CreationFailedError. | ||
| Using apiregistration API | ||
| Using yaml file api-service.yaml | ||
| """ |
There was a problem hiding this comment.
def test_create_apiservice_from_yaml_with_conflict(self):
"""
Creating the same apiservice twice should fail due to conflict.
"""
kubernetes/e2e_test/test_utils.py
Outdated
| with self.assertRaises(utils.FailToCreateError): |
There was a problem hiding this comment.
could you also verify the error message from apiserver?
| Using core, extensions API | ||
| Using yaml file list.yaml | ||
| """ |
There was a problem hiding this comment.
suggest
def test_create_from_yaml_list(self):
"""
Should be able to create multiple objects from a yaml of list kind.
"""
kubernetes/e2e_test/test_utils.py
Outdated
| namespace="default") | ||
| self.assertIsNotNone(svc) | ||
| ext_api = k8s_api[1] |
There was a problem hiding this comment.
I don't see the point of doing so. It's typed client anyway
it's not a good pattern to iterate through a list of arbitrary api objects and call typed api functions imo
micw523
force-pushed
the
yaml-list
branch
3 times, most recently
from
f3cd42c
to
35a1cdd
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: micw523, 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
|
Any plans to delete and apply? kubectl delete -f |
Fixes #671
This PR addresses #504 #671, which is an extension of #655.
It allows direct loading of YAMLs such as in
https://github.com/kubernetes/kubernetes/blob/master/hack/testdata/multi-resource-yaml.yaml
and
https://github.com/kubernetes/kubernetes/blob/master/hack/testdata/list.yaml
Important changes: