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.

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

rename FleetCluster to KubeConfigSecretRef #484

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

Xieql
Copy link
Contributor

@Xieql Xieql commented Nov 25, 2023

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

current FleetCluster struct is just for render

and we have already defined fleetCluster here

type fleetCluster struct {
Secret string
SecretKey string
client *kclient.Client
}

and we are going to make it public for #481

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Signed-off-by: Xieql <xieqianglong@huawei.com>
Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for kurator-dev canceled.

Name Link
🔨 Latest commit 5de1517
🔍 Latest deploy log https://app.netlify.com/sites/kurator-dev/deploys/6565d2e90c8546000815a345

@@ -81,7 +81,7 @@ func (f *FleetManager) reconcileBackupPlugin(ctx context.Context, fleet *v1alpha
// Iterating through each fleet cluster to generate and apply Velero helm configurations.
for key, cluster := range fleetClusters {
// generate Velero helm config for each fleet cluster
b, err := plugin.RenderVelero(f.Manifests, fleetNN, fleetOwnerRef, plugin.FleetCluster{
b, err := plugin.RenderVelero(f.Manifests, fleetNN, fleetOwnerRef, plugin.RenderableFleetCluster{
Copy link
Member

Choose a reason for hiding this comment

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

can we use the FleetCluster, if any field miss, we can add it

Copy link
Contributor Author

@Xieql Xieql Nov 27, 2023

Choose a reason for hiding this comment

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

the origin FleetCluster 's fileld value is comes from two struct fleetCluster and ClusterKey ,
so maybe we can not directly add fileld in FleetCluster, or we need merge those two struct into one struct, which may affect many other code

type fleetCluster struct {
Secret string
SecretKey string
client *kclient.Client
}
type ClusterKey struct {
Kind string
Name string
}

this is the most used usage:

b, err := plugin.RenderClusterStorage(f.Manifests, fleetNN, fleetOwnerRef, plugin.RenderableFleetCluster{
Name: key.Name,
SecretName: cluster.Secret,
SecretKey: cluster.SecretKey,

FleetCluster.Name:       ClusterKey.Name, 
FleetCluster.SecretName: fleetCluster.Secret, 
FleetCluster.SecretKey:  fleetCluster.SecretKey, 

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

Looked into the code closely, and the FleetCluster represents the kubeconfig secret ref.

We only need the secret name and key which kubeconfg is stored. Even the cluster name is not needed.

So we can rename it to KubeConfigSecretRef or SecretRef or something

Signed-off-by: Xieql <xieqianglong@huawei.com>
@Xieql
Copy link
Contributor Author

Xieql commented Nov 28, 2023

Looked into the code closely, and the FleetCluster represents the kubeconfig secret ref.

We only need the secret name and key which kubeconfg is stored. Even the cluster name is not needed.

So we can rename it to KubeConfigSecretRef or SecretRef or something

ok

@Xieql Xieql changed the title rename FleetCluster to RenderableFleetCluster rename FleetCluster to KubeConfigSecretRef Nov 28, 2023
@Xieql
Copy link
Contributor Author

Xieql commented Nov 28, 2023

/label tide/merge-method-squash

@@ -55,7 +55,7 @@ func (plugin FleetPluginConfig) StorageNamespace() string {
return plugin.Fleet.Namespace
}

type FleetCluster struct {
type KubeConfigSecretRef struct {
Name string
Copy link
Member

Choose a reason for hiding this comment

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

nit: The Name can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it is used solely for rendering and requires the name parameter.

If we remove name, we will need to refactor many corresponding functions

However, I think that refactoring them would not be wise.

such as

type FleetPluginConfig struct {
Name string
Component string
Fleet types.NamespacedName
OwnerReference *metav1.OwnerReference
Cluster *KubeConfigSecretRef
Chart ChartConfig
Values map[string]interface{}
}
func (plugin FleetPluginConfig) ResourceName() string {
if plugin.Cluster != nil {
return plugin.Component + "-" + plugin.Cluster.Name
}
return plugin.Component
}

Copy link
Member

Choose a reason for hiding this comment

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

I did see that, it is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, it is unused

@kurator-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

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

@Xieql Xieql force-pushed the refactor-fleetcluster branch 2 times, most recently from ff9026f to 5de1517 Compare November 28, 2023 11:45
@Xieql
Copy link
Contributor Author

Xieql commented Nov 28, 2023

If the name field is removed, the test fails with the message 'template: fleet plugin template:4:13: executing "fleet plugin template" at <$.ResourceName>: can't evaluate field ResourceName in type plugin.FleetPluginConfig'.

Therefore, it might be best to retain the field, as there seems to be no effective method to address this issue

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

/lgtm

@kurator-bot kurator-bot merged commit 984b852 into kurator-dev:main Nov 29, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants