✨ Cluster decorator interface #759
Conversation
And refactor creating to controller to call decorators Signed-off-by: Jian Qiu <jqiu@redhat.com>
Signed-off-by: Gaurav Jaswal <jaswalkiranavtar@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #759 +/- ##
==========================================
+ Coverage 63.58% 63.65% +0.06%
==========================================
Files 187 187
Lines 17989 17999 +10
==========================================
+ Hits 11439 11457 +18
+ Misses 5601 5594 -7
+ Partials 949 948 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Left some questions. Leaving it to @qiujian16 and other registration maintainers.
Part of #514
| @@ -71,6 +71,9 @@ type RegisterDriver interface { | |||
| // InformerHandler returns informer of the related object. If no object needs to be watched, the func could | |||
| // return nil, nil. | |||
| InformerHandler(option any) (cache.SharedIndexInformer, factory.EventFilterFunc) | |||
|
|
|||
| // ManagedClusterDecorator is to change managed cluster metadata or spec during registration process. | |||
| ManagedClusterDecorator(managedClusterArn string, managedClusterRoleSuffix string) ManagedClusterDecorator | |||
There was a problem hiding this comment.
in the interface level, the input should not be
managedClusterArn string, managedClusterRoleSuffix string
since other driver does not know about this, and if we have another driver later that needs other input, this would need to change the signature of the interface.
The interface should be the same as ManagedClusterDecorator, which is only to mutate the managedcluster.
The managedClusterArn and managedClusterRoleSuffix could be put into the driver at the NewDriver part.
pkg/registration/spoke/spokeagent.go
Outdated
| } | ||
| var registrationOption = o.registrationOption | ||
| if registrationOption.RegistrationAuth == AwsIrsaAuthType { | ||
| registerDriver = awsIrsa.NewAWSIRSADriver() |
There was a problem hiding this comment.
let's change this to
awsIrsa.NewAWSIRSADriver(o.registrationOption.ManagedClusterArn, o.registrationOption.ManagedClusterRoleSuffix)
so we do not need call decorator with these parameter.
Signed-off-by: Gaurav Jaswal <jaswalkiranavtar@gmail.com>
| @@ -230,7 +230,7 @@ func TestIsHubKubeConfigValidFunc(t *testing.T) { | |||
| } | |||
| for _, c := range cases { | |||
| t.Run(c.name, func(t *testing.T) { | |||
| driver := NewAWSIRSADriver() | |||
| driver := NewAWSIRSADriver("", "") | |||
There was a problem hiding this comment.
Please ignore this one, we are planning to fix this in future, when we implement the completion of joining, Did this now to resolve a compilation error.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jaswalkiranavtar, 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 |
openshift-merge-bot
bot
merged commit b170f3a
into
open-cluster-management-io:main
Summary
Adding ManagedCluster annotations using a Decorator
Related issue(s)
Fixes #