✨ Implement Init plugin subcommand #334

Fedosin · 2023-11-22T17:29:11Z

What this PR does / why we need it:
This PR adds Init subcommand to the operator plugin.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Fedosin · 2023-11-22T17:29:21Z

/hold

Fedosin · 2023-11-25T16:48:25Z

/test pull-cluster-api-operator-e2e-main

Fedosin · 2023-11-30T11:41:07Z

/hold cancel

Danil-Grigorev

Looks good, some nits, suggestions

Danil-Grigorev · 2024-01-15T12:29:36Z

cmd/plugin/cmd/init.go

 	bootstrapProviders      []string
 	controlPlaneProviders   []string
 	infrastructureProviders []string
 	// ipamProviders             []string


This can be uncommented now.

yes, I rebased this PR on master.

Danil-Grigorev · 2024-01-15T14:46:01Z

cmd/plugin/cmd/init.go


 func init() {
-	initCmd.PersistentFlags().StringVar(&initOpts.kubeconfig, "kubeconfig", "",
+	initCmd.PersistentFlags().StringVar(&initOpts.kubeconfig, "kubeconfig", filepath.Join(os.Getenv("HOME"), ".kube", "config"),


I think that by default empty kubeconfig should already resolve to ~/.kube/config, am I wrong?

I checked the utils method, it is not entirely correct, as it does not pick $KUBECONFIG variable, think it is supposed to.

Thank you for noticing this. Now we check KUBECONFIG env var as well.

Danil-Grigorev · 2024-01-15T14:48:54Z

cmd/plugin/cmd/init.go

 		"Add-on providers and versions (e.g. helm:v0.1.0) to add to the management cluster.")
 	initCmd.Flags().StringVarP(&initOpts.targetNamespace, "target-namespace", "n", "capi-operator-system",
 		"The target namespace where the operator should be deployed. If unspecified, the 'capi-operator-system' namespace is used.")
+	initCmd.Flags().StringVar(&initOpts.configSecretName, "config-secret-name", "",


Does it make sense to change configSecretName and configSeretNamespace to be a single flag for namespace/name, deserializing into reference, as it is in Provider spec? To be consistent with current api version.

converted these two options into 1: --config-secret, that has <config_secret_name>:<config_secret_namespace> format

Danil-Grigorev · 2024-01-15T14:52:04Z

cmd/plugin/cmd/init.go

+		"The config secret name to be used for the management cluster.")
+	initCmd.Flags().StringVar(&initOpts.configSecretNamespace, "config-secret-namespace", "capi-operator-system",
+		"The config secret namespace to be used for the management cluster.")
 	initCmd.Flags().BoolVar(&initOpts.waitProviders, "wait-providers", false,


It seems to me that waiting by default is a better choose for a CLI tool, WDYT?

Danil-Grigorev · 2024-01-15T15:11:05Z

cmd/plugin/cmd/init.go

+	return nil
+}
+
+func newGenericProvider(providerType clusterctlv1.ProviderType) operatorv1.GenericProvider {


We have a similar (but opposite of that) method in utils. Does it make sense to move it there? Also maybe we can leverage GetType on the provider wrapper to only store a list of providers

Moved this method to utils.go

Danil-Grigorev · 2024-01-15T15:22:57Z

cmd/plugin/cmd/init_test.go

+
+			for _, genericProvider := range tt.wantedProviders {
+				g.Eventually(func() (bool, error) {
+					provider, err := getGenericProvider(ctx, env, string(util.ClusterctlProviderType(genericProvider)), genericProvider.GetName(), genericProvider.GetNamespace())


BTW, you can define a custom interface deriving from client.Object and GenericProvider to get rid of the method and reduce it to a single client.Get:

type GeneriProviderObject { client.Object genericprovider.GenericProvider } … if err := client.Get(ctx, types.NamespacedName{Name: genericProvider.GetName(), Namespace: genericProvider.GetNamespace()}, provider); err != nil { return nil, err }

It says cannot use provider (variable of type GeneriProviderObject) as client.Object value in argument to env.Get: generiProviderObject does not implement client.Object (ambiguous selector generiProviderObject.DeepCopyObject). Maybe we can keep it for now?

Danil-Grigorev

/approve

One nit, otherwise looks good

Danil-Grigorev · 2024-01-23T15:18:27Z

internal/controller/phases.go

-	}
-
-	// if the url is a GitHub repository
-	if rURL.Host == githubDomain {


I think these constants can be removed now

httpsScheme = "https" githubDomain = "github.com" gitlabHostPrefix = "gitlab." gitlabPackagesAPIPrefix = "/api/v4/projects/"

k8s-ci-robot · 2024-01-23T15:20:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev

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:

~~OWNERS~~ [Danil-Grigorev]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alexander-demicev

/lgtm

k8s-ci-robot · 2024-01-24T16:18:27Z

LGTM label has been added.

Git tree hash: 325afaf4b34156d2df72350397933c59bdc6fd94

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 22, 2023

k8s-ci-robot requested review from fabriziopandini and timothysc November 22, 2023 17:29

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2023

Fedosin force-pushed the plugin_init branch 3 times, most recently from ee6eb9f to c596524 Compare November 24, 2023 16:04

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2023

Fedosin force-pushed the plugin_init branch 2 times, most recently from 8a9044b to 5839bb1 Compare November 24, 2023 16:16

Fedosin force-pushed the plugin_init branch 5 times, most recently from 2b4e873 to 706e246 Compare November 30, 2023 11:40

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 30, 2023

Fedosin force-pushed the plugin_init branch from 706e246 to 52cfe87 Compare December 5, 2023 14:29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2023

Fedosin force-pushed the plugin_init branch 2 times, most recently from 16699b6 to 306b24e Compare December 5, 2023 15:10

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2023

Fedosin force-pushed the plugin_init branch from 306b24e to cef91ac Compare December 18, 2023 12:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2023

Fedosin force-pushed the plugin_init branch from cef91ac to 7563ee0 Compare December 18, 2023 12:46

Fedosin force-pushed the plugin_init branch from 7563ee0 to 74cb9df Compare December 18, 2023 19:16

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2024

Danil-Grigorev reviewed Jan 15, 2024

View reviewed changes

Fedosin force-pushed the plugin_init branch 2 times, most recently from 755e726 to b9b1bb7 Compare January 22, 2024 15:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2024

Fedosin force-pushed the plugin_init branch from b9b1bb7 to 8819aac Compare January 23, 2024 14:25

Danil-Grigorev reviewed Jan 23, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2024

Danil-Grigorev mentioned this pull request Jan 23, 2024

✨ Plugin delete subcommand implementation #405

Merged

Fedosin added 2 commits January 24, 2024 11:39

Move RepositoryFactory function to common utils

262d394

Implement Init plugin subcommand

cd1cf47

Fedosin force-pushed the plugin_init branch from 8819aac to cd1cf47 Compare January 24, 2024 10:40

alexander-demicev approved these changes Jan 24, 2024

View reviewed changes

k8s-ci-robot assigned alexander-demicev Jan 24, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 24, 2024

k8s-ci-robot merged commit e3f4480 into kubernetes-sigs:main Jan 24, 2024

✨ Implement Init plugin subcommand #334

✨ Implement Init plugin subcommand #334

Uh oh!

Conversation

Fedosin commented Nov 22, 2023

Uh oh!

Fedosin commented Nov 22, 2023

Uh oh!

Fedosin commented Nov 25, 2023

Uh oh!

Fedosin commented Nov 30, 2023

Uh oh!

Danil-Grigorev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 23, 2024

Uh oh!

alexander-demicev left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants