-
Notifications
You must be signed in to change notification settings - Fork 1.8k
action: add delete #134
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
action: add delete #134
Conversation
Test cases: func main() {
samplePod := getSamplePod("Pod", "v1", "test-pod", "default")
fmt.Printf("Pod template: \n%s\n\n", getFormattedJSON(samplePod))
// Update non-existent pod
logrus.Infof("Test case 1: Delete a non-existent pod")
pod := samplePod
// Create pod
err := action.Delete(pod)
if err != nil {
logrus.Error(err)
} else {
logrus.Errorf("Deleted a non-existent pod")
fmt.Printf("Deleted Pod: \n%s\n\n", getFormattedJSON(pod))
}
// Delete existing pod
logrus.Infof("Test case 2: Delete an existing pod")
pod = samplePod
err = action.Create(pod)
if err != nil {
logrus.Errorf("Failed to create pod")
logrus.Fatal(err)
} else {
fmt.Printf("Created Pod: \n%s\n\n", getFormattedJSON(pod))
}
// Wait until pod is created and running
time.Sleep(time.Second * 10)
// Delete the pod with grace period 5s
metaDeleteOptions := &metav1.DeleteOptions{
GracePeriodSeconds: func(t int64) *int64 { return &t }(5),
}
err = action.Delete(pod, action.WithDeleteOptions(metaDeleteOptions))
if err != nil {
logrus.Errorf("Failed to delete pod")
logrus.Fatal(err)
}
logrus.Errorf("Successfully deleted pod")
} Test Logs:
Delete pod has the correct grace period set:
|
@fanminshi PTAL. |
pkg/sdk/action/api.go
Outdated
// Delete deletes the specified object | ||
// Returns an error if the object’s TypeMeta(Kind, APIVersion) or ObjectMeta(Name, Namespace) is missing or incorrect. | ||
// e.g NotFound https://github.com/kubernetes/apimachinery/blob/master/pkg/api/errors/errors.go#L418 | ||
// “opts” configures the k8s.io/apimachinery/pkg/apis/meta/v1.DeleteOptions |
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.
“opts” configures the k8s.io/apimachinery/pkg/apis/meta/v1.DeleteOptions
seems too specific. Also to configure v1.DeleteOptions
is just one possible of DeleteOption
s; Hence, i'd suggest to make it more generic.
Alternatively, if you want to be specific on what options are there, you can follow the KV comments from etcd:
// When passed WithRange(end), Get will return the keys in the range [key, end).
// When passed WithFromKey(), Get returns keys greater than or equal to key.
// When passed WithRev(rev) with rev > 0, Get retrieves keys at the given revision;
// if the required revision is compacted, the request will fail with ErrCompacted .
// When passed WithLimit(limit), the number of returned keys is bounded by limit.
// When passed WithSort(), the keys will be sorted.
https://github.com/coreos/etcd/blob/master/clientv3/kv.go#L42-L47
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.
True, let me add the etcd style when passed ...
comment.
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.
Yeah, I'll probably need to do that for Get and List operations
Test cases to consider:
|
Already tested that case in the above example.
Tested that as well and works fine. |
You are right. hmm, not sure how I missed those two cases from your testing output. |
lgtm |
…ating-.ci-operator.yaml-`build_root_image`-from-openshift/release Updating .ci-operator.yaml `build_root_image` from openshift/release
No description provided.