✨ Add importer into registraiton #753
Conversation
qiujian16
force-pushed
the
importer
branch
2 times, most recently
from
249dbc8 to
9ee219c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #753 +/- ##
==========================================
- Coverage 63.58% 63.46% -0.13%
==========================================
Files 187 192 +5
Lines 17989 18372 +383
==========================================
+ Hits 11439 11659 +220
- Misses 5601 5740 +139
- Partials 949 973 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jian Qiu <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
Signed-off-by: Jian Qiu <[email protected]>
Signed-off-by: Jian Qiu <[email protected]>
| } | ||
| } | ||
| if provider == nil { | ||
| logger.V(4).Info("provider not found for cluster", "cluster", cluster.Name) |
There was a problem hiding this comment.
| logger.V(4).Info("provider not found for cluster", "cluster", cluster.Name) | |
| logger.Info("provider not found for cluster", "cluster", cluster.Name) |
There was a problem hiding this comment.
maybe V(2), otherwise when we enable the controller, this logs will always be printed since it watches managedCluster
| const ( | ||
| operatorNamesapce = "open-cluster-management" | ||
| bootstrapSA = "cluster-bootstrap" | ||
| ConditionTypeImported = "Imported" |
There was a problem hiding this comment.
We have the condition ManagedClusterJoined and ManagedClusterConditionAvailable, consider changing this to ManagedClusterImported or ManagedClusterConditionImported
| result.Result, result.Changed, result.Error = resourceapply.ApplyDeployment( | ||
| ctx, clients.KubeClient.AppsV1(), recorder, t, 0) | ||
| results = append(results, result) | ||
| case *operatorv1.Klusterlet: |
There was a problem hiding this comment.
Here I don't quite understand why we handle the Deployment and Klusterlet separately, instead of using the resourceapply.ApplyDirectly?
There was a problem hiding this comment.
it only support ApplyDirectly only can apply certain static resource, deployment/klusterlet can do be applied with this func.
| // into the queue with the name of the managed cluster | ||
| Register(syncCtx factory.SyncContext) | ||
|
|
||
| // Run starts the provider |
There was a problem hiding this comment.
It is better to add more details for the Run comment, I don't know what I should do from the current comment to implement the Run.
|
|
||
| // Interface is the interface that a cluster provider should implement | ||
| type Interface interface { | ||
| // Clients is to return the client to connect to the target cluster. |
There was a problem hiding this comment.
Should we add some comments about what permission the returned client should have?
|
👍🏻 LGTM, left some nit comments. |
Summary
Related issue(s)
Fixes #
/hold