volcano icon indicating copy to clipboard operation
volcano copied to clipboard

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

Open googs1025 opened this issue 3 months ago • 13 comments

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 https://github.com/volcano-sh/volcano/issues/3053#issuecomment-2042837878

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

googs1025 avatar Apr 14 '24 13:04 googs1025

@lowang-bh /PTAL thank a lot!

googs1025 avatar Apr 17 '24 01:04 googs1025

@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

googs1025 avatar Apr 17 '24 14:04 googs1025

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!

lowang-bh avatar Apr 18 '24 03:04 lowang-bh

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign lowang-bh You can assign the PR to them by writing /assign @lowang-bh in a comment when ready.

The full list of commands accepted by this bot can be found 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 avatar Apr 18 '24 14:04 volcano-sh-bot

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 avatar Apr 18 '24 14:04 googs1025

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

googs1025 avatar Apr 18 '24 14:04 googs1025

done

googs1025 avatar Apr 21 '24 13:04 googs1025

/PTAL @william-wang @shinytang6 thanks a lot

googs1025 avatar Apr 28 '24 02:04 googs1025

New changes are detected. LGTM label has been removed.

volcano-sh-bot avatar May 07 '24 04:05 volcano-sh-bot