ROX-30941: Correct 'getDeduperAction' details#17108
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Please search and update any remaining references, including tests, to the old getDeduperAction name to prevent dangling usages.
- Add a unit test for TransitionTypeOpen2Open in getConnectionDeduperAction to explicitly verify it now returns deduperActionNoop.
- Remove or deprecate the old getDeduperAction implementation if it’s no longer in use, to avoid confusion going forward.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Please search and update any remaining references, including tests, to the old getDeduperAction name to prevent dangling usages.
- Add a unit test for TransitionTypeOpen2Open in getConnectionDeduperAction to explicitly verify it now returns deduperActionNoop.
- Remove or deprecate the old getDeduperAction implementation if it’s no longer in use, to avoid confusion going forward.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Did that - all clear.
This is an unexported implementation detail and I don't think that separate dedicated unit test coverage is necessary. This code is covered in other tests with larger scope.
This is not part of an exported contract, so no need for deprecating. |
|
Images are ready for the commit at bf2cc4a. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17108 +/- ##
==========================================
- Coverage 48.78% 48.75% -0.03%
==========================================
Files 2712 2712
Lines 202407 202403 -4
==========================================
- Hits 98746 98687 -59
- Misses 95876 95925 +49
- Partials 7785 7791 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@vikin91: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
|
/test gke-upgrade-tests |
Description
Clarify situation with 'deduperActionUpdateProcess' for connections.
Renames
getDeduperActionintogetConnectionDeduperActionto make clear that this function is used only in connection enrichment.Remove
case TransitionTypeOpen2OpenreturningdeduperActionUpdateProcessas it does not apply for connections.The correct deduper action for Open->Open transition is
deduperActionNoop.User-facing documentation
Testing and quality
Automated testing
How I validated my change
This change does not change behavior.
Unit tests are sufficient to validate that - those were run locally and on CI.