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

🐛 refactor gc controller #229

Merged
merged 1 commit into from
Oct 23, 2023
Merged

🐛 refactor gc controller #229

merged 1 commit into from
Oct 23, 2023

Conversation

zhiweiyin318
Copy link
Member

Summary

refactor rbacfinalizerdeletion to gccontroller, and delete work clustrerole after there is no cluster and manifestwork.

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from elgnay and skeeey July 24, 2023 10:38
@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 2 times, most recently from 8e682c9 to 8fbb20d Compare July 24, 2023 10:45
@codecov
Copy link

codecov bot commented Jul 24, 2023 •
edited
Loading

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (9aaa132) 61.15% compared to head (e21ae46) 61.36%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   61.15%   61.36%   +0.21%     
==========================================
  Files         129      131       +2     
  Lines       13661    13782     +121     
==========================================
+ Hits         8354     8458     +104     
- Misses       4547     4562      +15     
- Partials      760      762       +2
Flag Coverage Δ
unit 61.36% <74.39%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
pkg/registration/hub/clusterrole/controller.go 41.50% <100.00%> (ø)
pkg/registration/hub/managedcluster/controller.go 55.37% <100.00%> (+2.99%) ⬆️
pkg/registration/hub/gc/gc_resources.go 83.15% <83.15%> (ø)
pkg/registration/hub/gc/gc_cluster_rbac.go 77.02% <77.02%> (ø)
pkg/registration/hub/gc/controller.go 57.74% <57.74%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 2 times, most recently from 71b3810 to 8d989d8 Compare July 24, 2023 11:25
@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 2 times, most recently from e3eb1e6 to 3bf5e18 Compare July 27, 2023 10:20
@qiujian16
Copy link
Member

need some more ut for coverage.

qiujian16
qiujian16 reviewed Jul 27, 2023
View reviewed changes
pkg/registration/hub/gc/controller.go Outdated Show resolved Hide resolved
pkg/registration/hub/gc/controller.go Outdated Show resolved Hide resolved
pkg/registration/hub/gc/controller.go Outdated Show resolved Hide resolved
@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 2 times, most recently from a13309d to 4f453eb Compare July 27, 2023 13:03
@zhiweiyin318 zhiweiyin318 changed the title 🐛 refactor gc controller [wip]:bug: refactor gc controller Sep 6, 2023
@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 6 times, most recently from c366cff to d8eb6a2 Compare September 11, 2023 05:50
@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 6 times, most recently from d26684c to 53b808d Compare October 17, 2023 06:36
@qiujian16
Copy link
Member

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhiweiyin318

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

skeeey
skeeey reviewed Oct 18, 2023
View reviewed changes
pkg/registration/hub/gc/controller.go Outdated
clusterClient.ClusterV1().ManagedClusters())

gcResources := []schema.GroupVersionResource{}
if len(gcResourceList) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we give a default value to GCResourceList instead of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

the gcResourceList is from the flag gc-resource-list with the default values addon and manifetworks.

skeeey
skeeey reviewed Oct 18, 2023
View reviewed changes
pkg/registration/hub/gc/gc_resources.go
func stringToCleanupPriority(value string) cleanupPriority {
priority, err := strconv.Atoi(value)
if err != nil || cleanupPriority(priority) > maxCleanupPriority ||
cleanupPriority(priority) < minCleanupPriority {
Copy link
Member

Choose a reason for hiding this comment

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

nit: may need a warning message here

Copy link
Member Author

Choose a reason for hiding this comment

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

added.

@skeeey
Copy link
Member

skeeey commented Oct 18, 2023

LGTM

only have some nit comments, leave other reviewer to take a look

@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 5 times, most recently from b66c246 to 3330a20 Compare October 18, 2023 07:55
@zhiweiyin318
Copy link
Member Author

/hold

@zhiweiyin318 zhiweiyin318 force-pushed the work-role branch 3 times, most recently from fe1118e to dc74e8f Compare October 18, 2023 17:37
@zhiweiyin318
refactor gc controller
e21ae46 
Signed-off-by: Zhiwei Yin <zyin@redhat.com>
@zhiweiyin318
Copy link
Member Author

/unhold

@qiujian16
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 23, 2023
@openshift-ci openshift-ci bot merged commit f003ed3 into open-cluster-management-io:main Oct 23, 2023
12 checks passed
@zhiweiyin318 zhiweiyin318 deleted the work-role branch October 23, 2023 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skeeey skeeey skeeey left review comments

@qiujian16 qiujian16 qiujian16 left review comments

@elgnay elgnay Awaiting requested review from elgnay

Assignees

@qiujian16 qiujian16

@zhujian7 zhujian7

@elgnay elgnay

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zhiweiyin318 @qiujian16 @skeeey @openshift-merge-robot @zhujian7 @elgnay