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

add ut and refactor for pkg/scheduler/plugins/util/k8s package #3412

Merged
merged 2 commits into from
Jul 25, 2024
Merged

add ut and refactor for pkg/scheduler/plugins/util/k8s package #3412

merged 2 commits into from
Jul 25, 2024

Conversation

googs1025
Copy link
Member

@googs1025 googs1025 commented Apr 14, 2024 •
edited
Loading

What type of PR is this?

unit test

What this PR does / why we need it:

  • refactor nodelock
  • add some unit test for pkg/scheduler/plugins/util/k8s/snapshot.go
  • fix IsPVCUsedByPods method

Which issue(s) this PR fixes:

See more detail #3053 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

@volcano-sh-bot volcano-sh-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 14, 2024
@googs1025 googs1025 force-pushed the snapshot_ut branch 2 times, most recently from fd6cd8f to 671b661 Compare April 14, 2024 13:36
@googs1025
Copy link
Member Author

@lowang-bh /PTAL thank a lot!

lowang-bh
lowang-bh reviewed Apr 17, 2024
View reviewed changes
Copy link
Member

@lowang-bh lowang-bh left a comment

Choose a reason for hiding this comment

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

/lgtm

By the way, what's ut coverage percentage?

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2024
@googs1025
Copy link
Member Author

googs1025 commented Apr 17, 2024 •
edited
Loading

@lowang-bh
image
image
Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two
https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79
https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

@lowang-bh
Copy link
Member

Additionally, I noticed some code duplication in this section. I intend to submit a refactoring pull request to improve it here. like those two https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L79 https://github.com/volcano-sh/volcano/blob/master/pkg/scheduler/plugins/util/nodelock/nodelock.go#L112

WDYT

Welcom！

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2024
@googs1025
Copy link
Member Author

In addition to adding unit tests, I also tried to refactor nodelock. Because it involves changes to the original logic, please reviewers to help me check it carefully.
/PTAL @lowang-bh @william-wang @shinytang6

@googs1025
Copy link
Member Author

/assign @william-wang @shinytang6
Thanks for the review

@googs1025 googs1025 changed the title add ut for pkg/scheduler/plugins/util/k8s package add ut and refactor for pkg/scheduler/plugins/util/k8s package Apr 20, 2024
@volcano-sh-bot volcano-sh-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 20, 2024
@volcano-sh-bot volcano-sh-bot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 20, 2024
@googs1025 googs1025 requested a review from hwdef May 7, 2024 02:04
@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label May 7, 2024
@googs1025 googs1025 requested a review from lowang-bh May 8, 2024 00:43
@googs1025 googs1025 requested a review from lowang-bh May 8, 2024 14:32
@googs1025 googs1025 force-pushed the snapshot_ut branch 2 times, most recently from 673650c to 3187e1c Compare May 9, 2024 01:34
@googs1025
Copy link
Member Author

@Monokaix @lowang-bh Please help review this pr thanks!

@lowang-bh
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2024
@googs1025 googs1025 mentioned this pull request Jun 4, 2024
Closed
@googs1025
Copy link
Member Author

@william-wang /PTAL thanks!

@googs1025
Copy link
Member Author

Are there any suggested changes or can we move on to the next step? @william-wang @Monokaix

@volcano-sh-bot volcano-sh-bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024
@googs1025
refecotr: nodelock
1a7c18a 
Signed-off-by: googs1025 <[email protected]>
@Monokaix
Copy link
Member

/lgtm

@volcano-sh-bot volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 24, 2024
hwdef
hwdef reviewed Jul 10, 2024
View reviewed changes
Copy link
Member

@hwdef hwdef left a comment

Choose a reason for hiding this comment

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

/lgtm

william-wang
william-wang approved these changes Jul 25, 2024
View reviewed changes
Copy link
Member

@william-wang william-wang left a comment

Choose a reason for hiding this comment

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

/approve

@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@volcano-sh-bot volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024
@volcano-sh-bot volcano-sh-bot merged commit 4925061 into volcano-sh:master Jul 25, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Monokaix Monokaix Monokaix left review comments

@hwdef hwdef hwdef left review comments

@william-wang william-wang william-wang approved these changes

@shinytang6 shinytang6 Awaiting requested review from shinytang6

@lowang-bh lowang-bh Awaiting requested review from lowang-bh

Assignees

@william-wang william-wang

@hwdef hwdef

@Monokaix Monokaix

@lowang-bh lowang-bh

@shinytang6 shinytang6

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@googs1025 @lowang-bh @Monokaix @volcano-sh-bot @william-wang @hwdef @shinytang6