Repository: knative/pkg
Pull request #2670 (2023-01-23 16:09:22)

Make SetDefaults of Destination duck type nil safer
To use `SetDefaults` on the `Destination` type, the caller must check that the destination is not nil currently:

e.g.
```
if subscriber != nil {
    subscriber.SetDefaults(ctx)
}
```

Some other `SetDefaults` implementations, check this internally to prevent panics. E.g.: https://github.com/knative/eventing/blob/7e899fd166deb40c885d3fc4fb9e2901375a7bff/pkg/apis/duck/v1/delivery_defaults.go#L22-L24
This makes it easier to directly call `SetDefaults` without surrounding it with `nil` checks.

This PR addresses it and includes the "nil check" into the `SetDefaults` methods of a `Destination`

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2670#" title="Author self-approved">creydr</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign vaikas for approval by writing `/assign @vaikas` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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

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

- **[apis/duck/OWNERS](https://github.com/knative/pkg/blob/main/apis/duck/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":["vaikas"]} -->
creydr: /assign @pierDipi 
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2670#" title="Author self-approved">creydr</a>*, *<a href="https://github.com/knative/pkg/pull/2670#pullrequestreview-1265897053" title="LGTM">pierDipi</a>*
**Once this PR has been reviewed and has the lgtm label**, please assign vaikas for approval by writing `/assign @vaikas` in a comment. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

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

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

- **[apis/duck/OWNERS](https://github.com/knative/pkg/blob/main/apis/duck/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":["vaikas"]} -->
pierDipi: Can we add a unit test ?
knative-prow[bot]: New changes are detected. LGTM label has been removed.
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2670?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
Base: **81.36**% // Head: **81.36**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`9ecc009`)](https://codecov.io/gh/knative/pkg/pull/2670?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) compared to base [(`247510c`)](https://codecov.io/gh/knative/pkg/commit/247510c00e9dbb7f5a23b76e8ab76654de173a04?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main    #2670   +/-   ##
=======================================
  Coverage   81.36%   81.36%           
=======================================
  Files         162      162           
  Lines        9846     9849    +3     
=======================================
+ Hits         8011     8014    +3     
  Misses       1594     1594           
  Partials      241      241           
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [apis/duck/v1/destination.go](https://codecov.io/gh/knative/pkg/pull/2670?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL2Rlc3RpbmF0aW9uLmdv) | `100.00% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2670?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

creydr: /assign @vaikas
vaikas: Thanks!
/lgtm
/approve
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2670#" title="Author self-approved">creydr</a>*, *<a href="https://github.com/knative/pkg/pull/2670#pullrequestreview-1265897053" title="LGTM">pierDipi</a>*, *<a href="https://github.com/knative/pkg/pull/2670#issuecomment-1403259344" title="Approved">vaikas</a>*

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

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:

- ~~[apis/duck/OWNERS](https://github.com/knative/pkg/blob/main/apis/duck/OWNERS)~~ [vaikas]

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":[]} -->

Patch:
diff --git a/apis/duck/v1/destination.go b/apis/duck/v1/destination.go
index 87f0983fd..c895e6d29 100644
--- a/apis/duck/v1/destination.go
+++ b/apis/duck/v1/destination.go
@@ -73,6 +73,10 @@ func (d *Destination) GetRef() *KReference {
 }
 
 func (d *Destination) SetDefaults(ctx context.Context) {
+	if d == nil {
+		return
+	}
+
 	if d.Ref != nil && d.Ref.Namespace == "" {
 		d.Ref.Namespace = apis.ParentMeta(ctx).Namespace
 	}


Test patch:
diff --git a/apis/duck/v1/destination_test.go b/apis/duck/v1/destination_test.go
index 6472c6162..a514e2207 100644
--- a/apis/duck/v1/destination_test.go
+++ b/apis/duck/v1/destination_test.go
@@ -179,7 +179,10 @@ func TestDestinationSetDefaults(t *testing.T) {
 		d    *Destination
 		ctx  context.Context
 		want string
-	}{"uri set, nothing in ref, not modified ": {
+	}{"destination nil ": {
+		d:   nil,
+		ctx: ctx,
+	}, "uri set, nothing in ref, not modified ": {
 		d:   &Destination{URI: apis.HTTP("example.com")},
 		ctx: ctx,
 	}, "namespace set, nothing in context, not modified ": {
@@ -206,11 +209,13 @@ func TestDestinationSetDefaults(t *testing.T) {
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
 			tc.d.SetDefaults(tc.ctx)
-			if tc.d.Ref != nil && tc.d.Ref.Namespace != tc.want {
-				t.Errorf("Got: %s wanted %s", tc.d.Ref.Namespace, tc.want)
-			}
-			if tc.d.Ref == nil && tc.want != "" {
-				t.Error("Got: nil Ref wanted", tc.want)
+			if tc.d != nil {
+				if tc.d.Ref != nil && tc.d.Ref.Namespace != tc.want {
+					t.Errorf("Got: %s wanted %s", tc.d.Ref.Namespace, tc.want)
+				}
+				if tc.d.Ref == nil && tc.want != "" {
+					t.Error("Got: nil Ref wanted", tc.want)
+				}
 			}
 		})
 	}