-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Delete from yaml #1197
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
Delete from yaml #1197
Conversation
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. |
Welcome @DiptoChakrabarty! |
Hey @DiptoChakrabarty |
Yes I have done it |
kubernetes/utils/delete_from_yaml.py
Outdated
print(msg) | ||
|
||
|
||
class FailToCreateError(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be FailToDeleteError?
kubernetes/utils/delete_from_yaml.py
Outdated
# convert group name from DNS subdomain format to | ||
# python class name convention | ||
group = "".join(word.capitalize() for word in group.split('.')) | ||
group = "".join(word.capitalize() for word in group.split('.')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks the two lines above are the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes resolving these small issues
kubernetes/utils/delete_from_yaml.py
Outdated
from kubernetes import client | ||
|
||
|
||
def delete_from_yaml( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, what is the difference between this and create_from_yaml? could you please list the differences in the PR description? is it possible to reuse to avoid dups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I will do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have written the difference between the methods create_from_yaml and delete_from_yaml editing the PR description and also fixed the issues as stated earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add some e2e testcases in https://github.com/kubernetes-client/python/blob/master/kubernetes/e2e_test/test_utils.py and update https://github.com/kubernetes-client/python/blob/master/kubernetes/utils/__init__.py
@@ -0,0 +1,146 @@ | |||
import re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add copyright boilerplate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure Ill add the boilerplate and try writing the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added the boilerplate and added tests also modifying delete_from_dict function
kubernetes/utils/delete_from_yaml.py
Outdated
this parameter has no effect. | ||
Available parameters for creating <kind>: | ||
:param async_req bool | ||
:param bool include_uninitialized: If true, partially initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the initializer feature has been removed
kubernetes/utils/delete_from_yaml.py
Outdated
API object (i.e. List, Service, etc). | ||
Input: | ||
k8s_client: an ApiClient object, initialized with the client args. | ||
data: a dictionary holding valid kubernetes objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment doesn't match the parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed the comment with correct parameter
kubernetes/utils/delete_from_yaml.py
Outdated
name = yml_document["metadata"]["name"] | ||
#call function to delete from namespace | ||
res = getattr(k8s_api,"delete_namespaced_{}".format(kind))( | ||
name=name,body=client.V1DeleteOptions(propagation_policy="Foreground", grace_period_seconds=5),**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want to achieve parity with kubectl delete -f
, we should probably use Background policy by default with a Cascade option: https://github.com/kubernetes/kubernetes/blob/0c642b6ef01071219b9b9e34ad6d89cb9b4b1971/staging/src/k8s.io/kubectl/pkg/cmd/delete/delete.go#L304-L308
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have updated this to use background policy
kubernetes/utils/delete_from_yaml.py
Outdated
name = yml_document["metadata"]["name"] | ||
kwargs.pop('namespace', None) | ||
res = getattr(k8s_api,"delete_{}".format(kind))( | ||
name=name,body=client.V1DeleteOptions(propagation_policy="Foreground", grace_period_seconds=5),**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A 200 response from a delete request doesn't guarantee the resource being deleted. We should wait until the resource is no longer visible from the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should the e2e tests work for objects being deleted , after deletion if they are search (the object) it gets timed out resulting in an error ,however in the create_from_yaml tests in the end all resources are being deleted using multiple separate commands could we delete those using the delete_from_yaml method and that would be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign @yliaog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in e2e, after delete, try listing the object again, if it does not exist anymore, it means the object is successfully deleted.
@DiptoChakrabarty @roycaihw - Does the API support non-namespaced resources such as
I feel they should also be supported. Same seems to be the case with
Adding a check |
/hold (for resolving the issues with the commit history in this branch) |
hey can you tell me how can I resolve this commit history issue . |
Can you please merge? |
Hey can I send a new PR again with the same content , then can close this PR |
@DiptoChakrabarty Could you open a new PR? That would help with rebasing and squashing the commits. Thanks! |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DiptoChakrabarty The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Looks like this is superseded by #1392. Closing this. Please reopen if it's not the case. |
/close |
@palnabarun: Closed this PR. 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. |
This is a method to delete kubernetes resources using the yaml files . It is similar to the method create_from_yaml but for deleting kubernetes resources .
It can be used to any type pf resource deployment , pod , service etc.
Fixes #940
Resolved issue for tests failing for python2.7
create_from_yaml creates kubernetes objects like deployments,serivces,ingress etc from the given yml files , the delete_from_yaml method can be used to remove/delete those objects from the same yml files in the given cluster for any namespace