-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
remove check for downloadrequest #5440
remove check for downloadrequest #5440
Conversation
Signed-off-by: cleverhu <shouping.hu@daocloud.io>
6df2fa4
to
ef7d851
Compare
Codecov Report
@@ Coverage Diff @@
## main #5440 +/- ##
=======================================
Coverage 40.62% 40.62%
=======================================
Files 236 236
Lines 20449 20449
=======================================
Hits 8308 8308
Misses 11532 11532
Partials 609 609 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
lgtm, thanks
if updated.Name != created.Name { | ||
return | ||
} | ||
|
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'm not quite sure I understand the relationship between this check and the issue referenced. The issue seems to be about adding fieldSelector support for CRDs, but where is Velero's use of field selectors dependent on the above check?
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.
According to the pr #3004 (comment),
Pretty sure we can actually remove these checks now. However, let's not in this PR :)
There is a need to clean up the code.
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.
This is a forgotten child issue.
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 saw that. But I'm still unclear how these things are related. I don't doubt that this is likely unnecessary now, but I'm just not seeing how the change referenced there relates to whether or not we need this. How is this check related to the field selectors changed referenced in http://issue.k8s.io/51046
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closing the stale issue. |
Thank you for contributing to Velero!
Please add a summary of your change
remove check for downloadrequest
Does your change fix a particular issue?
Fixes #3126
Please indicate you've done the following:
/kind changelog-not-required
as a comment on this pull request.site/content/docs/main
.