fix: fix checkObjectNamespaceLabels func param type #2477
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2477 +/- ##
==========================================
- Coverage 64.71% 64.66% -0.06%
==========================================
Files 115 115
Lines 17431 17470 +39
==========================================
+ Hits 11281 11297 +16
- Misses 5433 5451 +18
- Partials 717 722 +5 ☔ View full report in Codecov by Sentry. |
|
@zirain The rest of the files cannot be covered by the test, do I need to deal with it? |
Yes, I have fix it |
|
/retest |
| @@ -580,8 +580,10 @@ func (r *gatewayAPIReconciler) findReferenceGrant(ctx context.Context, from, to | |||
| if len(r.namespaceLabels) != 0 { | |||
| var rgs []gwapiv1b1.ReferenceGrant | |||
| for _, refGrant := range refGrants { | |||
| refGrant := refGrant | |||
|
|
|||
| ns := refGrant.GetNamespace() | |||
There was a problem hiding this comment.
In principle, we don’t need ns here, but I considered the future scalability and may still use this ns, so I kept it.
There was a problem hiding this comment.
if thats the case, do we need to change the signature of checkObjectNamespaceLabels, how are we benefitting from it ?
There was a problem hiding this comment.
First of all, I think that the step of obtaining the namespace in advance before calling the function may be redundant externally. We should move this step inside the function and let the function do this step for us.
Secondly, we may expand the judgment conditions in the future, using the interface form of metav1.object as a parameter, which will allow us to obtain more information.
Finally, subsequent modifications to this function will not affect external calls.
There was a problem hiding this comment.
vote for removing these ns, since there's no other particular usage. if we encounter this in the future, we can simply change it back.
There was a problem hiding this comment.
Yes, of course we can do that. One reason why I didn't remove ns is because the subsequent error message used the ns variable. I didn't want to modify the original code too much. I thought that this ns variable might be used in the future, so I made it a retained variable. rather than removing it.
@shawnh2
There was a problem hiding this comment.
imo, removing these ns is kinda point of this PR. as for those used in error msg, directly using xxx.GetNamespace() works for me.
| func (r *gatewayAPIReconciler) checkObjectNamespaceLabels(obj matav1.Object) (bool, error) { | ||
|
|
||
| var nsString string | ||
| // it requires extra condition? |
There was a problem hiding this comment.
Strictly speaking, this may be a TODO. Because we need to judge cluster resources and some resources that do not have namespace.
There was a problem hiding this comment.
got it! can you make this comment more clear? it will help those who come across it later.
There was a problem hiding this comment.
yes, i have do that
Signed-off-by: ShyunnY <1147212064@qq.com>
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fix the checkObjectNamespaceLabels function param type so that it can support passing in common interface types. Avoid obtaining namespace in advance outside the function.
Fixes #