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

Specify the provider manually. #1025

Merged
merged 1 commit into from
Nov 25, 2021

Conversation

2hangchen
Copy link
Contributor

@2hangchen 2hangchen commented Nov 24, 2021

Signed-off-by: Pilipalaca [email protected]

What type of PR is this?
/kind feature

What this PR does / why we need it:
In the clusterlist,provider is a display column,so it should be manuall specified when join a member cluster.After manually specifiying the provider,you can get the specified provider throuh apiserver.
Which issue(s) this PR fixes:
Fixes #1023

Special notes for your reviewer:
image
Join with provider,then get it from apiserver:
image
Does this PR introduce a user-facing change?:

`kubectl-karmada` & `karmadactl`: Introduced `--cluster-provider` flag to `join` command to specify provider of joining cluster. 

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 24, 2021
@karmada-bot karmada-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 24, 2021
@2hangchen
Copy link
Contributor Author

The process is done,can you help review,thank you.
/assign @RainbowMango

@RainbowMango
Copy link
Member

OK. on it. thanks.

@@ -134,6 +137,7 @@ func (j *CommandJoinOption) AddFlags(flags *pflag.FlagSet) {
"Context name of cluster in kubeconfig. Only works when there are multiple contexts in the kubeconfig.")
flags.StringVar(&j.ClusterKubeConfig, "cluster-kubeconfig", "",
"Path of the cluster's kubeconfig.")
flags.StringVar(&j.ClusterProvider, "cluster-provider", "", "Cluster provider.")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
flags.StringVar(&j.ClusterProvider, "cluster-provider", "", "Cluster provider.")
flags.StringVar(&j.ClusterProvider, "cluster-provider", "", "Provider of the joining cluster.")

@@ -278,7 +282,7 @@ func generateSecretInMemberCluster(clusterKubeClient kubeclient.Interface, clust
return clusterSecret, nil
}

func generateClusterInControllerPlane(controlPlaneConfig, clusterConfig *rest.Config, clusterName string, secret corev1.Secret) (*clusterv1alpha1.Cluster, error) {
func generateClusterInControllerPlane(controlPlaneConfig, clusterConfig *rest.Config, clusterName string, provider string, secret corev1.Secret) (*clusterv1alpha1.Cluster, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I concern the parameter list is a little bit too long, can we use the CommandJoinOption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too,in my own branch,in addition to 'provider',i also added a lot of fields,using in CommandJoinOption struct.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to change this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can use CommandJoinOption to pass parameter in this case.

Copy link
Member

Choose a reason for hiding this comment

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

+1

@RainbowMango
Copy link
Member

cc @lonelyCZ

@lonelyCZ
Copy link
Member

Thank you @2hangchen , could you squash your commits to one?

@@ -303,7 +311,7 @@ func generateClusterInControllerPlane(controlPlaneConfig, clusterConfig *rest.Co
controlPlaneKarmadaClient := karmadaclientset.NewForConfigOrDie(controlPlaneConfig)
cluster, err := CreateClusterObject(controlPlaneKarmadaClient, clusterObj, false)
if err != nil {
return nil, fmt.Errorf("failed to create cluster object. cluster name: %s, error: %v", clusterName, err)
return nil, fmt.Errorf("failed to create cluster object. cluster name: %s, error: %v", opts.ClusterProvider, err)
Copy link
Member

Choose a reason for hiding this comment

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

It should be opts.ClusterName

@2hangchen
Copy link
Contributor Author

Ok, I'm working on it

Signed-off-by: Pilipalaca <[email protected]>

change the params

Signed-off-by: Pilipalaca <[email protected]>
@lonelyCZ
Copy link
Member

Thanks, @2hangchen

lgtm

@RainbowMango
Copy link
Member

/approve

Leave LGTM to @lonelyCZ (for try the robot command)

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2021
@lonelyCZ
Copy link
Member

Thanks, @RainbowMango

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2021
@karmada-bot karmada-bot merged commit 76c7514 into karmada-io:master Nov 25, 2021
@2hangchen 2hangchen deleted the feat_providerwithjoin branch May 9, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is it possible to add the provider field when managing the cluster?
4 participants