-
Notifications
You must be signed in to change notification settings - Fork 95
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.
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
✨ Starting aws registration by spoke by assuming IAM role on startup and adding annotations to ManagedCluster CR #714
Conversation
81fc233
to
259ab9a
Compare
67c342a
to
a41f367
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #714 +/- ##
==========================================
+ Coverage 63.41% 63.43% +0.01%
==========================================
Files 185 186 +1
Lines 17802 17843 +41
==========================================
+ Hits 11290 11318 +28
- Misses 5581 5591 +10
- Partials 931 934 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
/assign @qiujian16 |
Hi @zhujian7 @zhiweiyin318 @dongbeiqing91 Bulk of the changes in this PR is related to the operator. Could you PTAL whenever you have some cycles? Thanks. |
…d adding annotations to ManagedCluster CR Signed-off-by: Erica Jin <132393634+EricaJ6@users.noreply.github.com>
a41f367
to
430d3ff
Compare
Signed-off-by: Erica Jin <132393634+EricaJ6@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thank you for your contribution!
Left some comments. Regarding showing the error on klusterlet condition, maybe wait for the operator maintainers and/or Qiu Jian's feedback.
manifests/klusterlet/managed/klusterlet-registration-serviceaccount.yaml
Outdated
Show resolved
Hide resolved
manifests/klusterlet/managed/klusterlet-registration-serviceaccount.yaml
Outdated
Show resolved
Hide resolved
manifests/klusterlet/managed/klusterlet-work-serviceaccount.yaml
Outdated
Show resolved
Hide resolved
manifests/klusterlet/managed/klusterlet-work-serviceaccount.yaml
Outdated
Show resolved
Hide resolved
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Erica Jin <132393634+EricaJ6@users.noreply.github.com>
Addressed comments other than adding failure conditions. Please show us a sample on how to add failure condition. |
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
pkg/operator/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Erica Jin <132393634+EricaJ6@users.noreply.github.com>
Thanks for approving api repo PR. Do you have any further comments on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this PR!
Huge thank you to all the code reviewers for taking the time to provide valuable feedback. I’ve ensured that all your comments have been addressed. If there are any additional concerns, @jaswalkiranavtar and his team will address them in another PR quickly.
Typically, I am not the one to approve PRs involving this area of the codebase. However, I’m making an exception here as, based on my review, all changes are contained within the hub on EKS associated flags. This containment minimizes the risk of regression to the main/latest branch. To accommodate the contributors' schedules and planning, I’m approving this merge based on the prior reviews, comments, and my own assessment.
/approve
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed the code and comments. /LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaswalkiranavtar, jnpacker, mikeshng 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 |
93db6de
into
open-cluster-management-io:main
Summary
This PR contains
Related issue(s)
#514