Skip to content
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

backport test: Refactor mock CSI manager into release/1.5.x #18561

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

gulducat
Copy link
Member

manual backport of #18554 - 7bd5c6e to 1.5.x

auto backport should work for 1.4.x from here.

and MockCSIManager to support the call counting
that csi_hook_test expects

instead of implementing csimanager
interfaces in two separate places:
* client/allocrunner/csi_hook_test
* client/csi_endpoint_test

they can both use the same mocks defined in
client/pluginmanager/csimanager/
alongside the actual implementations of them.

also refactor TestCSINode_DetachVolume
to use use it like Node_ExpandVolume
so we can also test the happy path there
@gulducat gulducat added the backport/1.4.x backport to 1.4.x release line label Sep 22, 2023
@gulducat gulducat requested a review from tgross September 22, 2023 21:33
@gulducat gulducat changed the title backport test: Refactor mock CSI manager backport test: Refactor mock CSI manager into release/1.5.x Sep 22, 2023
Copy link
Contributor

@pkazmierczak pkazmierczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I give a conditional LGTM as long as we all agree we're backporting changes like this. My understanding was that up until now these sort of changes weren't backported, but I guess having them in previous releases makes other backports less prone to conflicts?

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I give a conditional LGTM as long as we all agree we're backporting changes like this. My understanding was that up until now these sort of changes weren't backported, but I guess having them in previous releases makes other backports less prone to conflicts?

You're totally right @pkazmierczak that we generally only backport bug fixes. But yeah, having test code like this makes our future backports less painful, without breaking the promises we make to users (that you typically only get new features in the new versions).

@gulducat gulducat merged commit 66c8cbb into release/1.5.x Sep 25, 2023
22 checks passed
@gulducat gulducat deleted the backport-csi-1.5.x branch September 25, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.4.x backport to 1.4.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants