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.

Sign up for GitHub

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

Jump to bottom

Implement/fix the unit tests of Volume Attachment and volume controller #2053

Merged
merged 2 commits into from
Jul 14, 2023
Merged

Implement/fix the unit tests of Volume Attachment and volume controller #2053

merged 2 commits into from
Jul 14, 2023

Conversation

PhanLe1010
Copy link
Contributor

@PhanLe1010 PhanLe1010 requested a review from a team as a code owner July 7, 2023 03:43
@PhanLe1010
Copy link
Contributor Author

@mergify backport v1.5.x

@mergify
Copy link

mergify bot commented Jul 7, 2023 •
edited
Loading

backport v1.5.x

✅ Backports have been created

james-munson
james-munson previously approved these changes Jul 7, 2023
View reviewed changes
Copy link
Contributor

@james-munson james-munson left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

controller/volume_attachment_controller_test.go Show resolved Hide resolved
ejweber
ejweber requested changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

One potential blocker, but a quick fix if one is needed.

I have some questions about changes in volume_controller_test.go, but they are more for my understanding than anything. I will sync up with you offline.

controller/volume_attachment_controller_test.go Show resolved Hide resolved
controller/volume_controller_test.go Outdated Show resolved Hide resolved
shuo-wu
shuo-wu reviewed Jul 12, 2023
View reviewed changes
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

I think it's better to add 2 more kinds of tests for the VA controller:

  1. Generation related tests
  2. Attachment/detachment cases caused by higher priority tickets suddenly introducing.

controller/volume_attachment_controller_test.go Show resolved Hide resolved
controller/volume_attachment_controller_test.go Show resolved Hide resolved
controller/volume_attachment_controller_test.go Show resolved Hide resolved
controller/volume_attachment_controller_test.go Show resolved Hide resolved
@PhanLe1010
Copy link
Contributor Author

I think it's better to add 2 more kinds of tests for the VA controller:

  1. Generation related tests
  2. Attachment/detachment cases caused by higher priority tickets suddenly introducing.

Thank you. Added

@PhanLe1010 PhanLe1010 requested a review from shuo-wu July 13, 2023 01:39
ejweber
ejweber reviewed Jul 13, 2023
View reviewed changes
Copy link
Contributor

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

The changes I asked for were made. Curious on a couple of Shuo's questions.

shuo-wu
shuo-wu previously approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

In general LGTM

ejweber
ejweber previously approved these changes Jul 14, 2023
View reviewed changes
@PhanLe1010
Fix volume_controller unit tests
d9f4bb9 
Longhorn-6005

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
Add unit tests for volumeattachment_controller
103224d 
Longhorn-6005

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010 PhanLe1010 dismissed stale reviews from ejweber and shuo-wu via 103224d July 14, 2023 21:07
ejweber
ejweber approved these changes Jul 14, 2023
View reviewed changes
Copy link
Contributor

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

LGTM

@PhanLe1010 PhanLe1010 merged commit c59efbf into longhorn:master Jul 14, 2023
3 checks passed
@mergify mergify bot mentioned this pull request Jul 14, 2023
Merged
@PhanLe1010 PhanLe1010 mentioned this pull request Jul 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejweber ejweber ejweber approved these changes

@james-munson james-munson james-munson approved these changes

@shuo-wu shuo-wu shuo-wu left review comments

@derekbit derekbit Awaiting requested review from derekbit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PhanLe1010 @ejweber @shuo-wu @james-munson