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

Support Reserve plugin #82

Merged
merged 17 commits into from
Dec 22, 2023
Merged

Support Reserve plugin #82

merged 17 commits into from
Dec 22, 2023

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Dec 5, 2023

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

Support Reserve plugin

Which issue(s) this PR fixes:

Part of #72

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Support postBind plugin

What are the benchmark results of this change?

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 5, 2023
@utam0k
Copy link
Member Author

utam0k commented Dec 5, 2023

I'll fix it tomorrow.

utam0k added 3 commits December 7, 2023 21:15
@utam0k
make lint
7a08a9b 
Signed-off-by: utam0k <[email protected]>
@utam0k
Fix test
58c9463 
Signed-off-by: utam0k <[email protected]>
@utam0k
make check
51cbfa2 
Signed-off-by: utam0k <[email protected]>
@utam0k
Copy link
Member Author

utam0k commented Dec 7, 2023

@sanposhiho @codefromthecrypt This PR is ready for your review 🙏

@sanposhiho
Copy link
Member

/assign
/assign @Gekko0114

Gekko0114
Gekko0114 reviewed Dec 8, 2023
View reviewed changes
guest/reserve/reserve.go Outdated Show resolved Hide resolved
@Gekko0114
Copy link
Member

@utam0k
Overall looks good! Left minor comments.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2023
@Gekko0114
Copy link
Member

sanposhiho fixed the issue. So this failure will be fixed.
#86

@utam0k
Copy link
Member Author

utam0k commented Dec 17, 2023

@Gekko0114 Don't worry, thanks for your look 🙏

Or maybe this function causes this issue...??
I will take a look when I have time, sorry for not having a clear answer

@utam0k utam0k requested a review from Gekko0114 December 17, 2023 12:57
@Gekko0114
Copy link
Member

@utam0k
Thanks! Left minor comments.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2023
@utam0k
Address the feedbacks
459a5a1 
Signed-off-by: utam0k <[email protected]>
@utam0k
Fix conflict
d72195b 
Signed-off-by: utam0k <[email protected]>
@utam0k
Fix test
adf2061 
Signed-off-by: utam0k <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 20, 2023
@utam0k utam0k requested a review from Gekko0114 December 20, 2023 12:16
@utam0k
go mod tidy
3a6658f 
Signed-off-by: utam0k <[email protected]>
@Gekko0114
Copy link
Member

Gekko0114 commented Dec 20, 2023 •
edited
Loading

@utam0k
Overall looks good!
Sorry, I left one comment about typo.

@utam0k
Copy link
Member Author

utam0k commented Dec 21, 2023 •
edited
Loading

@Gekko0114 Did you forget to submit your review? I didn't find your comment 😭

Sorry, I left one comment about typo.

Gekko0114
Gekko0114 reviewed Dec 21, 2023
View reviewed changes
scheduler/test/testdata/error/panic_on_reserve.wat Outdated
(unreachable)
)

;; We require exporting unresere with reserve
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
;; We require exporting unresere with reserve
;; We require exporting unreserve with reserve

guest/testdata/cyclestate/main.go Outdated Show resolved Hide resolved
scheduler/test/testdata/error/panic_on_reserve.wat Outdated
(unreachable)
)

;; We require exporting unresere with reserve
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
;; We require exporting unresere with reserve
;; We require exporting unreserve with reserve

@Gekko0114
Copy link
Member

@utam0k
So sorry, seems I didn't submit my comments. Please let me know if you can't see it even now.

sanposhiho
sanposhiho approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

/approve

Leave /lgtm to @Gekko0114

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanposhiho, utam0k

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2023
@utam0k
Fix typo
426e4e5 
Signed-off-by: utam0k <[email protected]>
@utam0k utam0k requested a review from Gekko0114 December 22, 2023 12:45
@Gekko0114
Copy link
Member

/lgtm

Thank you so much!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2023
@k8s-ci-robot k8s-ci-robot merged commit 73a62c8 into kubernetes-sigs:main Dec 22, 2023
4 checks passed
@sanposhiho sanposhiho mentioned this pull request Jan 5, 2024
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanposhiho sanposhiho sanposhiho approved these changes

@codefromthecrypt codefromthecrypt Awaiting requested review from codefromthecrypt

@Gekko0114 Gekko0114 Awaiting requested review from Gekko0114

Assignees

@Gekko0114 Gekko0114

@sanposhiho sanposhiho

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", 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.
4 participants
@utam0k @sanposhiho @Gekko0114 @k8s-ci-robot