New function development -- yurtctl adds parameter enable app manager to control automatic deployment of yurtappmanager. #352

yanyhui · 2021-06-12T02:12:48Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage
/kind feature

What this PR does / why we need it:

New function development.

Which issue(s) this PR fixes:

Fixes #340

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot · 2021-06-12T02:12:55Z

@yanyhui: GitHub didn't allow me to assign the following users: your_reviewer.

Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:
/kind bug
/kind documentation
/kind enhancement
/kind good-first-issue
/kind feature
/kind question
/kind design
/sig ai
/sig iot
/sig network
/sig storage
/sig storage
/kind feature

What this PR does / why we need it:

New function development.

Which issue(s) this PR fixes:

Fixes #340

Special notes for your reviewer:

Does this PR introduce a user-facing change?
other Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yanyhui · 2021-06-12T06:21:23Z

This function has been verified successfully.

rambohe-ch · 2021-06-15T02:01:53Z

@yanyhui Please fix golangci-lint error

rambohe-ch · 2021-06-15T02:02:16Z

/assign @kadisi @Peeknut

Peeknut · 2021-06-17T02:45:56Z

pkg/yurtctl/cmd/convert/convert.go

@@ -38,6 +38,7 @@ import (
 	"github.com/openyurtio/openyurt/pkg/yurtctl/lock"
 	kubeutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/kubernetes"
 	strutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/strings"
+	"k8s.io/client-go/dynamic"


import order needs to be modified

Peeknut · 2021-06-17T02:50:42Z

pkg/yurtctl/cmd/convert/convert.go

+	cmd.Flags().BoolP("enable-app-manager", "e", false,
+		"if set, yurtappmanager will be deployed.")
+	cmd.Flags().String("yurt-app-manager-image",
+		"openyurt/yurt-app-manager:v0.4.0",


I think use tag latest is better.

The latest version image can't be used without updating. Now we all use the version of v0.4.0.

Peeknut · 2021-06-17T03:05:14Z

pkg/yurtctl/cmd/convert/convert.go

@@ -353,6 +380,18 @@ func (co *ConvertOptions) RunConvert() (err error) {
 	if err != nil {
 		return err
 	}
+
+	//8. deploy the yurtappmanager if required


The codes before and after "deploy the yurtappmanager" all belong to step7. How about move it behind step6?

Step 7 and "deploy the yurtappmanager" are separate processes and have no impact, so I don't think we need to put them before step 7.

Peeknut · 2021-06-17T03:09:34Z

pkg/yurtctl/util/kubernetes/util.go

@@ -50,6 +50,14 @@ import (
 	"github.com/openyurtio/openyurt/pkg/yurtctl/constants"
 	strutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/strings"
 	tmplutil "github.com/openyurtio/openyurt/pkg/yurtctl/util/templates"
+	"bytes"


import order needs to be modified

Peeknut · 2021-06-17T03:21:37Z

pkg/yurtctl/cmd/convert/convert.go

@@ -377,6 +416,84 @@ func (co *ConvertOptions) RunConvert() (err error) {
 	return
 }

+func deployYurtAppManager(
+	client *kubernetes.Clientset,
+	cloudNodes []string,


The parameter cloudNodes is not used and can be removed.

This is a good idea. I found that this parameter was not used at that time.

Peeknut · 2021-06-17T05:37:01Z

pkg/yurtctl/util/kubernetes/util.go

+func CreateCRDFromYaml(clientset *kubernetes.Clientset, yurtAppManagerClient dynamic.Interface, nameSpace string,filebytes []byte) error {
+	var err error
+	decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewReader(filebytes), 10000)
+	for {


Why use a for loop？

Peeknut · 2021-06-17T05:38:03Z

pkg/yurtctl/util/kubernetes/util.go

+	for {
+		var rawObj runtime.RawExtension
+		if err = decoder.Decode(&rawObj); err != nil {
+			break


Why use break instead of return err?

Peeknut · 2021-06-17T06:00:11Z

pkg/yurtctl/util/kubernetes/util.go

+		}
+
+		mapper := restmapper.NewDiscoveryRESTMapper(gr)
+		mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)


When was the gvk information of crd registered?

Before creating CRD.

Peeknut · 2021-06-17T06:08:39Z

pkg/yurtctl/cmd/convert/convert.go

+
+	// 1.create the YurtAppManagerCustomResourceDefinition
+	// 1.1 nodepool
+	if err := kubeutil.CreateCRDFromYaml(client, yurtAppManagerClient, "",[]byte(constants.YurtAppManagerNodePool)); err != nil {


It seems like the func CreateCRDFromYaml is to create a cr. However, it's used to register crd here.

register crd

Peeknut · 2021-06-17T06:11:51Z

pkg/yurtctl/util/kubernetes/util.go

@@ -357,6 +504,23 @@ func GenClientSet(flags *pflag.FlagSet) (*kubernetes.Clientset, error) {
 	return kubernetes.NewForConfig(restCfg)
 }

+//add by yanyhui at 20210611


It is better to delete the comment.

Peeknut · 2021-06-18T12:52:37Z

/lgtm

kadisi · 2021-06-21T06:06:30Z

/approve

thanks @yanyhui

openyurt-bot · 2021-06-21T06:06:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadisi, yanyhui

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~~ [kadisi]

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

… to control automatic deployment of yurtappmanager. (openyurtio#352) * yurtctl增加参数enable-app-manager控制自动部署yurtappmanager * 代码规范修改 * 代码规范修改golangci

yurtctl增加参数enable-app-manager控制自动部署yurtappmanager

483d56f

openyurt-bot added the kind/feature kind/feature label Jun 12, 2021

openyurt-bot requested review from charleszheng44 and huangyuqi June 12, 2021 02:12

openyurt-bot added the size/XXL label Jun 12, 2021

openyurt-bot assigned kadisi and Peeknut Jun 15, 2021

Peeknut reviewed Jun 17, 2021

View reviewed changes

yanyhui added 2 commits June 18, 2021 11:02

代码规范修改

7ca2b32

代码规范修改golangci

e378b2d

openyurt-bot added the lgtm lgtm label Jun 18, 2021

Peeknut mentioned this pull request Jun 20, 2021

[REQUEST Membership]: New membership for Peeknut: reviewer of openyurtio/openyurt #362

Closed

openyurt-bot added the approved approved label Jun 21, 2021

openyurt-bot merged commit ced11b7 into openyurtio:master Jun 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New function development -- yurtctl adds parameter enable app manager to control automatic deployment of yurtappmanager. #352

New function development -- yurtctl adds parameter enable app manager to control automatic deployment of yurtappmanager. #352

yanyhui commented Jun 12, 2021

openyurt-bot commented Jun 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

yanyhui commented Jun 12, 2021

rambohe-ch commented Jun 15, 2021

rambohe-ch commented Jun 15, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut Jun 17, 2021

yanyhui Jun 18, 2021

Peeknut commented Jun 18, 2021

kadisi commented Jun 21, 2021

openyurt-bot commented Jun 21, 2021

New function development -- yurtctl adds parameter enable app manager to control automatic deployment of yurtappmanager. #352

New function development -- yurtctl adds parameter enable app manager to control automatic deployment of yurtappmanager. #352

Conversation

yanyhui commented Jun 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

openyurt-bot commented Jun 12, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

other Note

yanyhui commented Jun 12, 2021

rambohe-ch commented Jun 15, 2021

rambohe-ch commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Peeknut commented Jun 18, 2021

kadisi commented Jun 21, 2021

openyurt-bot commented Jun 21, 2021