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

🌱 set cma managed by addon-manager if not configured #374

Merged
merged 3 commits into from
Mar 25, 2024
Merged

🌱 set cma managed by addon-manager if not configured #374

merged 3 commits into from
Mar 25, 2024

Conversation

haoqing0110
Copy link
Member

@haoqing0110 haoqing0110 commented Mar 12, 2024 •
edited
Loading

Summary

Ref: #355

Related issue(s)

Fixes #

@openshift-ci openshift-ci bot requested review from zhiweiyin318 and zhujian7 March 12, 2024 08:28
@haoqing0110
Copy link
Member Author

/hold
More testing cases to add.

@haoqing0110 haoqing0110 changed the title [WIP] set cma managed by addon-manager if not configured [WIP] 🌱 set cma managed by addon-manager if not configured Mar 12, 2024
@codecov Codecov
Copy link

codecov bot commented Mar 12, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 61.48%. Comparing base (76aa752) to head (c324213).
Report is 4 commits behind head on main.

Files Patch % Lines
pkg/addon/controllers/cmamanagedby/controller.go 87.87% 4 Missing ⚠️
...addon/controllers/addonconfiguration/controller.go 0.00% 1 Missing ⚠️
...on/controllers/cmainstallprogression/controller.go 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #374      +/-   ##
==========================================
+ Coverage   61.37%   61.48%   +0.11%     
==========================================
  Files         132      133       +1     
  Lines       14051    14087      +36     
==========================================
+ Hits         8624     8662      +38     
  Misses       4672     4672              
+ Partials      755      753       -2
Flag Coverage Δ
unit 61.48% <84.21%> (+0.11%) ⬆️

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

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

@haoqing0110
set cma managed by addon-manager if not configured
ab035c1 
Signed-off-by: haoqing0110 <qhao@redhat.com>
@openshift-ci openshift-ci bot removed the approved label Mar 19, 2024
@haoqing0110 haoqing0110 force-pushed the br_addon-manager branch 3 times, most recently from 4c4c965 to 19d6335 Compare March 20, 2024 08:04
@haoqing0110
update annotation in testing case
6adeddb 
Signed-off-by: haoqing0110 <qhao@redhat.com>
@haoqing0110 haoqing0110 changed the title [WIP] 🌱 set cma managed by addon-manager if not configured 🌱 set cma managed by addon-manager if not configured Mar 20, 2024
@haoqing0110
Copy link
Member Author

/assign @qiujian16

qiujian16
qiujian16 reviewed Mar 21, 2024
View reviewed changes
pkg/addon/controllers/managementaddon/controller.go Outdated
)

// clusterManagementAddonController reconciles cma on the hub.
type clusterManagementAddonController struct {
Copy link
Member

Choose a reason for hiding this comment

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

need a better name and a better description on what this controller really does.

Copy link
Member Author

Choose a reason for hiding this comment

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

@qiujian16 will add more description, for the name, any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

If the purpose is to set annotation, maybe just CMAManagedByController

Copy link
Member Author

Choose a reason for hiding this comment

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

Have modified the controller and added more description.

@haoqing0110 haoqing0110 mentioned this pull request Mar 21, 2024
Merged
@haoqing0110
rename controller name
c324213 
Signed-off-by: haoqing0110 <qhao@redhat.com>
qiujian16
qiujian16 reviewed Mar 21, 2024
View reviewed changes
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve

/assign @zhujian7

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@zhujian7
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 25, 2024
@haoqing0110
Copy link
Member Author

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 22501d8 into open-cluster-management-io:main Mar 25, 2024
13 checks passed
@haoqing0110 haoqing0110 deleted the br_addon-manager branch March 25, 2024 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiujian16 qiujian16 qiujian16 left review comments

@zhiweiyin318 zhiweiyin318 Awaiting requested review from zhiweiyin318

@zhujian7 zhujian7 Awaiting requested review from zhujian7

Assignees

@qiujian16 qiujian16

@zhujian7 zhujian7

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haoqing0110 @zhujian7 @qiujian16