Repository: knative/client
Pull request #805 (2020-04-15 08:18:40)

Check DeleteTimestamp before updating resource
Fixes: #786 

## Description
* Check DeleteTimestamp before updating resource. Include: service, apiserver source, sink binding, ping source, trigger.

Release Note:
- 🐛 Bug fix
 Check DeleteTimestamp before updating resource.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/805#" title="Author self-approved">MIBc</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **navidshaikh**
You can assign the PR to them by writing `/assign @navidshaikh` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["navidshaikh"]} -->
knative-prow-robot: Hi @MIBc. Thanks for your PR.

I'm waiting for a [knative](https://github.com/orgs/knative/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/knative/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

rhuss: /ok-to-test
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/kn/commands/trigger/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1250366040416718848/artifacts/line-cov.html#file1) | 80.4% | 81.1% | 0.7
[pkg/serving/v1/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1250366040416718848/artifacts/line-cov.html#file2) | 69.1% | 68.4% | -0.8

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/805#pullrequestreview-394176726" title="Approved">maximilien</a>*, *<a href="https://github.com/knative/client/pull/805#" title="Author self-approved">MIBc</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/master/OWNERS)~~ [maximilien]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/kn/commands/source/binding/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251062127171473409/artifacts/line-cov.html#file1) | 74.4% | 75.6% | 1.1
[pkg/kn/commands/source/ping/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251062127171473409/artifacts/line-cov.html#file2) | 75.0% | 76.3% | 1.3
[pkg/kn/commands/trigger/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251062127171473409/artifacts/line-cov.html#file3) | 80.4% | 81.1% | 0.7
[pkg/serving/v1/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251062127171473409/artifacts/line-cov.html#file4) | 69.1% | 68.4% | -0.8

knative-prow-robot: @MIBc: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-unit-tests | cd27fe51b4e98b25c452e22ee2c06c58d0c596eb | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-unit-tests/1251062127171473411) | `/test pull-knative-client-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_client/805). [Your PR dashboard](https://gubernator.knative.dev/pr/MIBc).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
MIBc: /retest
knative-prow-robot: @MIBc: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-client-unit-tests | cd27fe51b4e98b25c452e22ee2c06c58d0c596eb | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-unit-tests/1251064015174504449) | `/test pull-knative-client-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_client/805). [Your PR dashboard](https://gubernator.knative.dev/pr/MIBc).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/kn/commands/source/apiserver/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251067538272423939/artifacts/line-cov.html#file1) | 73.2% | 74.4% | 1.2
[pkg/kn/commands/source/binding/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251067538272423939/artifacts/line-cov.html#file2) | 74.4% | 75.6% | 1.1
[pkg/kn/commands/source/ping/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251067538272423939/artifacts/line-cov.html#file3) | 75.0% | 76.3% | 1.3
[pkg/kn/commands/trigger/update.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251067538272423939/artifacts/line-cov.html#file4) | 80.4% | 81.1% | 0.7
[pkg/serving/v1/client.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/805/pull-knative-client-go-coverage/1251067538272423939/artifacts/line-cov.html#file5) | 69.1% | 68.4% | -0.8


Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 6b59a9d1..626667ab 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -22,6 +22,10 @@
 | Add `-a` flag as an alias for `--annotation`
 | https://github.com/knative/client/pull/782[#782]
 
+| 🐛
+| Check DeleteTimestamp before updating resource
+| https://github.com/knative/client/pull/805[#805]
+
 |===
 
 ## v0.13.2 (2020-04-15)
@@ -80,7 +84,7 @@
 
 | 🐛
 | Fix default config path on Windows
-| https://github.com/knative/client/pull/751[#751]
+| https://github.com/knative/client/pull/752[#752]
 |===
 
 ## v0.13.0 (2020-03-11)
diff --git a/pkg/kn/commands/source/apiserver/update.go b/pkg/kn/commands/source/apiserver/update.go
index 146dc1cf..6ec97bc0 100644
--- a/pkg/kn/commands/source/apiserver/update.go
+++ b/pkg/kn/commands/source/apiserver/update.go
@@ -63,6 +63,9 @@ func NewAPIServerUpdateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
+			if source.GetDeletionTimestamp() != nil {
+				return fmt.Errorf("can't update apiserver source %s because it has been marked for deletion", name)
+			}
 
 			b := v1alpha2.NewAPIServerSourceBuilderFromExisting(source)
 			if cmd.Flags().Changed("service-account") {
diff --git a/pkg/kn/commands/source/binding/update.go b/pkg/kn/commands/source/binding/update.go
index 93f490f4..b8b8526f 100644
--- a/pkg/kn/commands/source/binding/update.go
+++ b/pkg/kn/commands/source/binding/update.go
@@ -61,6 +61,9 @@ func NewBindingUpdateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
+			if source.GetDeletionTimestamp() != nil {
+				return fmt.Errorf("can't update binding %s because it has been marked for deletion", name)
+			}
 
 			b := v1alpha12.NewSinkBindingBuilderFromExisting(source)
 			if cmd.Flags().Changed("sink") {
diff --git a/pkg/kn/commands/source/ping/update.go b/pkg/kn/commands/source/ping/update.go
index 60ea0918..80ce941d 100644
--- a/pkg/kn/commands/source/ping/update.go
+++ b/pkg/kn/commands/source/ping/update.go
@@ -61,6 +61,9 @@ func NewPingUpdateCommand(p *commands.KnParams) *cobra.Command {
 			if err != nil {
 				return err
 			}
+			if source.GetDeletionTimestamp() != nil {
+				return fmt.Errorf("can't update ping source %s because it has been marked for deletion", name)
+			}
 
 			b := v1alpha2.NewPingSourceBuilderFromExisting(source)
 			if cmd.Flags().Changed("schedule") {
diff --git a/pkg/kn/commands/trigger/update.go b/pkg/kn/commands/trigger/update.go
index fa5a036e..2f6313b6 100644
--- a/pkg/kn/commands/trigger/update.go
+++ b/pkg/kn/commands/trigger/update.go
@@ -75,6 +75,9 @@ func NewTriggerUpdateCommand(p *commands.KnParams) *cobra.Command {
 				if err != nil {
 					return err
 				}
+				if trigger.GetDeletionTimestamp() != nil {
+					return fmt.Errorf("can't update trigger %s because it has been marked for deletion", name)
+				}
 
 				b := client_v1alpha1.NewTriggerBuilderFromExisting(trigger)
 
diff --git a/pkg/serving/v1/client.go b/pkg/serving/v1/client.go
index a4bb53a1..ca2d0f59 100644
--- a/pkg/serving/v1/client.go
+++ b/pkg/serving/v1/client.go
@@ -237,6 +237,9 @@ func updateServiceWithRetry(cl KnServingClient, name string, updateFunc serviceU
 		if err != nil {
 			return err
 		}
+		if service.GetDeletionTimestamp() != nil {
+			return fmt.Errorf("can't update service %s because it has been marked for deletion", name)
+		}
 		updatedService, err := updateFunc(service.DeepCopy())
 		if err != nil {
 			return err


Test patch:
diff --git a/pkg/kn/commands/service/update_test.go b/pkg/kn/commands/service/update_test.go
index 9364ff79..888f6832 100644
--- a/pkg/kn/commands/service/update_test.go
+++ b/pkg/kn/commands/service/update_test.go
@@ -20,6 +20,7 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	"gotest.tools/assert/cmp"
@@ -726,6 +727,16 @@ func TestServiceUpdateNoClusterLocalOnPrivateService(t *testing.T) {
 	assert.DeepEqual(t, expected, actual)
 }
 
+func TestServiceUpdateDeletionTimestampNotNil(t *testing.T) {
+	original := newEmptyService()
+	original.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	_, _, _, err := fakeServiceUpdate(original, []string{
+		"service", "update", "foo", "--revision-name", "foo-v1"})
+	assert.ErrorContains(t, err, original.Name)
+	assert.ErrorContains(t, err, "deletion")
+	assert.ErrorContains(t, err, "service")
+}
+
 func newEmptyService() *servingv1.Service {
 	ret := &servingv1.Service{
 		TypeMeta: metav1.TypeMeta{
diff --git a/pkg/kn/commands/source/apiserver/update_test.go b/pkg/kn/commands/source/apiserver/update_test.go
index 3b6bf807..2b0d9ee4 100644
--- a/pkg/kn/commands/source/apiserver/update_test.go
+++ b/pkg/kn/commands/source/apiserver/update_test.go
@@ -16,6 +16,7 @@ package apiserver
 
 import (
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 
@@ -48,3 +49,17 @@ func TestApiServerSourceUpdate(t *testing.T) {
 
 	apiServerRecorder.Validate()
 }
+
+func TestApiServerSourceUpdateDeletionTimestampNotNil(t *testing.T) {
+	apiServerClient := v1alpha2.NewMockKnAPIServerSourceClient(t)
+	apiServerRecorder := apiServerClient.Recorder()
+
+	present := createAPIServerSource("testsource", "Event", "v1", "testsa1", "Ref", "svc1", false)
+	present.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	apiServerRecorder.GetAPIServerSource("testsource", present, nil)
+
+	_, err := executeAPIServerSourceCommand(apiServerClient, nil, "update", "testsource", "--service-account", "testsa2", "--sink", "svc:svc2")
+	assert.ErrorContains(t, err, present.Name)
+	assert.ErrorContains(t, err, "deletion")
+	assert.ErrorContains(t, err, "apiserver")
+}
diff --git a/pkg/kn/commands/source/binding/update_test.go b/pkg/kn/commands/source/binding/update_test.go
index b7c006fa..fba37abc 100644
--- a/pkg/kn/commands/source/binding/update_test.go
+++ b/pkg/kn/commands/source/binding/update_test.go
@@ -17,6 +17,7 @@ package binding
 import (
 	"errors"
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -66,3 +67,16 @@ func TestUpdateError(t *testing.T) {
 
 	bindingRecorder.Validate()
 }
+
+func TestBindingUpdateDeletionTimestampNotNil(t *testing.T) {
+	sinkBindingClient := clientsourcesv1alpha1.NewMockKnSinkBindingClient(t)
+	bindingRecorder := sinkBindingClient.Recorder()
+	present := createSinkBinding("testbinding", "", deploymentGvk, "", nil)
+	present.DeletionTimestamp = &v1.Time{Time: time.Now()}
+	bindingRecorder.GetSinkBinding("testbinding", present, nil)
+
+	_, err := executeSinkBindingCommand(sinkBindingClient, nil, "update", "testbinding")
+	assert.ErrorContains(t, err, present.Name)
+	assert.ErrorContains(t, err, "deletion")
+	assert.ErrorContains(t, err, "binding")
+}
diff --git a/pkg/kn/commands/source/ping/update_test.go b/pkg/kn/commands/source/ping/update_test.go
index ca3de723..59db0b9a 100644
--- a/pkg/kn/commands/source/ping/update_test.go
+++ b/pkg/kn/commands/source/ping/update_test.go
@@ -17,6 +17,7 @@ package ping
 import (
 	"errors"
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -74,3 +75,16 @@ func TestUpdateError(t *testing.T) {
 
 	pingRecorder.Validate()
 }
+
+func TestPingUpdateDeletionTimestampNotNil(t *testing.T) {
+	pingSourceClient := clientv1alpha2.NewMockKnPingSourceClient(t)
+	present := createPingSource("test", "", "", "")
+	present.DeletionTimestamp = &v1.Time{Time: time.Now()}
+	pingRecorder := pingSourceClient.Recorder()
+	pingRecorder.GetPingSource("test", present, nil)
+
+	_, err := executePingSourceCommand(pingSourceClient, nil, "update", "test")
+	assert.ErrorContains(t, err, present.Name)
+	assert.ErrorContains(t, err, "deletion")
+	assert.ErrorContains(t, err, "ping")
+}
diff --git a/pkg/kn/commands/trigger/update_test.go b/pkg/kn/commands/trigger/update_test.go
index eb953f57..6aab9bcb 100644
--- a/pkg/kn/commands/trigger/update_test.go
+++ b/pkg/kn/commands/trigger/update_test.go
@@ -17,6 +17,7 @@ package trigger
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"gotest.tools/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -74,3 +75,18 @@ func TestTriggerUpdateInvalidBroker(t *testing.T) {
 
 	eventingRecorder.Validate()
 }
+
+func TestTriggerUpdateDeletionTimestampNotNil(t *testing.T) {
+	eventingClient := clienteventingv1alpha1.NewMockKnEventingClient(t)
+
+	eventingRecorder := eventingClient.Recorder()
+	present := createTrigger("default", triggerName, map[string]string{"type": "dev.knative.foo"}, "mybroker", "mysvc")
+	present.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+	eventingRecorder.GetTrigger(triggerName, present, nil)
+
+	_, err := executeTriggerCommand(eventingClient, nil, "update", triggerName,
+		"--filter", "type=dev.knative.new", "--sink", "svc:mysvc")
+	assert.ErrorContains(t, err, present.Name)
+	assert.ErrorContains(t, err, "deletion")
+	assert.ErrorContains(t, err, "trigger")
+}