[GitHub] [skywalking-infra-e2e] kezhenxu94 commented on a change in pull request #3: Add setup and wait according to e2e.yaml

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #3:
URL: https://github.com/apache/skywalking-infra-e2e/pull/3#discussion_r567671931



##########
File path: commands/setup/setup.go
##########
@@ -15,43 +15,48 @@
 // KIND, either express or implied.  See the License for the
 // specific language governing permissions and limitations
 // under the License.
+
 package setup
 
 import (
 	"fmt"
 
+	"github.com/apache/skywalking-infra-e2e/internal/config"
+
 	"github.com/apache/skywalking-infra-e2e/internal/constant"
 
 	"github.com/spf13/cobra"
 
 	"github.com/apache/skywalking-infra-e2e/internal/components/setup"
 	"github.com/apache/skywalking-infra-e2e/internal/logger"
-	"github.com/apache/skywalking-infra-e2e/internal/util"
-
-	"github.com/apache/skywalking-infra-e2e/internal/flags"
 )
 
-func init() {
-	Setup.Flags().StringVar(&flags.Env, "env", "kind", "specify test environment")
-	Setup.Flags().StringVar(&flags.File, "file", "kind.yaml", "specify configuration file")
-}
-
 var Setup = &cobra.Command{
 	Use:   "setup",
 	Short: "",
 	RunE: func(cmd *cobra.Command, args []string) error {
-		if flags.Env == constant.Compose {
-			if util.Which(constant.ComposeCommand) != nil {
-				return fmt.Errorf("command %s not found in the PATH", constant.ComposeCommand)
-			}
-			logger.Log.Info("env for docker-compose not implemented")
-		} else if flags.Env == constant.Kind {
-			if err := setup.KindSetupInCommand(); err != nil {
-				return err
-			}
-		} else {
-			return fmt.Errorf("no such env for setup: [%s]. should use kind or compose instead", flags.Env)
+		err := setupAccordingE2E()
+		if err != nil {
+			err = fmt.Errorf("[Setup] %s", err)
+			return err
 		}
 		return nil
 	},
 }
+
+func setupAccordingE2E() error {
+	e2eConfig := config.GlobalConfig.E2EConfig
+
+	if e2eConfig.Setup.Env == constant.Kind {
+		err := setup.KindSetup(&e2eConfig)
+		if err != nil {
+			return err
+		}
+	} else if e2eConfig.Setup.Env == constant.Compose {
+		logger.Log.Info("env for docker-compose not implemented")

Review comment:
       Return an error early to avoid unnecessary further operations

##########
File path: internal/components/cleanup/kind.go
##########
@@ -28,29 +31,37 @@ import (
 	"strings"
 
 	"github.com/apache/skywalking-infra-e2e/internal/constant"
-	"github.com/apache/skywalking-infra-e2e/internal/flags"
 	"github.com/apache/skywalking-infra-e2e/internal/logger"
 )
 
-var (
-	kindConfigFile string
-)
-
 type KindClusterNameConfig struct {
 	Name string
 }
 
-func KindCleanupInCommand() error {
-	kindConfigFile = flags.File
+func KindCleanUp(e2eConfig *config.E2EConfig) error {
+	kindConfigFilePath := e2eConfig.Setup.File
+
+	k8sConfigFile := constant.K8sClusterConfigFile
 
-	if err := cleanKindCluster(); err != nil {
+	logger.Log.Infof("deleting k8s cluster config file:%s", k8sConfigFile)
+	err := os.Remove(k8sConfigFile)
+	if err != nil {
+		logger.Log.Errorf("delete k8s cluster config file failed")
 		return err
 	}
+
+	logger.Log.Info("deleting kind cluster...")
+	if err := cleanKindCluster(kindConfigFilePath); err != nil {
+		logger.Log.Error("delete kind cluster failed")
+		return err
+	}
+	logger.Log.Info("delete kind cluster succeeded")

Review comment:
       Swap this 2 parts, delete the cluster first, then delete the `k8sConfigFile`

##########
File path: internal/components/setup/kind.go
##########
@@ -19,32 +19,83 @@
 package setup
 
 import (
+	"fmt"
+	"io/ioutil"
+	"os"
 	"strings"
+	"time"
+
+	"k8s.io/cli-runtime/pkg/genericclioptions"
+	ctlwait "k8s.io/kubectl/pkg/cmd/wait"
+
+	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/kubernetes"
+
+	"github.com/apache/skywalking-infra-e2e/internal/config"
+
+	apiv1 "k8s.io/api/admission/v1"
+
+	"github.com/apache/skywalking-infra-e2e/internal/util"
 
 	kind "sigs.k8s.io/kind/cmd/kind/app"
 	kindcmd "sigs.k8s.io/kind/pkg/cmd"
 
 	"github.com/apache/skywalking-infra-e2e/internal/constant"
 
-	"github.com/apache/skywalking-infra-e2e/internal/flags"
 	"github.com/apache/skywalking-infra-e2e/internal/logger"
 )
 
 var (
 	kindConfigFile string
+	kubeConfigPath string
 )
 
-func KindSetupInCommand() error {
-	kindConfigFile = flags.File
+// KindSetup sets up environment according to e2e.yaml.
+func KindSetup(e2eConfig *config.E2EConfig) error {
+	kindConfigFile = e2eConfig.Setup.File
+
+	timeout := e2eConfig.Setup.Timeout
+	var waitTimeout time.Duration
+	if timeout == 0 {

Review comment:
       ```suggestion
   	if timeout <= 0 {
   ```

##########
File path: internal/components/setup/kind.go
##########
@@ -54,3 +105,112 @@ func createKindCluster() error {
 	logger.Log.Info("create kind cluster succeeded")
 	return nil
 }
+
+// createManifestsAndWait creates manifests in k8s cluster and concurrent waits according to the manifests' wait conditions.
+func createManifestsAndWait(c *kubernetes.Clientset, dc dynamic.Interface, manifests []config.Manifest, timeout time.Duration) error {
+	waitSet := util.NewWaitSet(timeout)
+
+	kubeConfigYaml, err := ioutil.ReadFile(kubeConfigPath)
+	if err != nil {
+		return err
+	}
+
+	for idx := range manifests {
+		manifest := manifests[idx]
+		waits := manifest.Waits
+		err := createByManifest(c, dc, manifest)
+		if err != nil {
+			return err
+		}
+
+		if waits == nil {

Review comment:
       ```suggestion
   		if waits == nil || len(waits) == 0 {
   ```

##########
File path: internal/components/setup/kind.go
##########
@@ -54,3 +105,112 @@ func createKindCluster() error {
 	logger.Log.Info("create kind cluster succeeded")
 	return nil
 }
+
+// createManifestsAndWait creates manifests in k8s cluster and concurrent waits according to the manifests' wait conditions.
+func createManifestsAndWait(c *kubernetes.Clientset, dc dynamic.Interface, manifests []config.Manifest, timeout time.Duration) error {
+	waitSet := util.NewWaitSet(timeout)
+
+	kubeConfigYaml, err := ioutil.ReadFile(kubeConfigPath)
+	if err != nil {
+		return err
+	}
+
+	for idx := range manifests {
+		manifest := manifests[idx]
+		waits := manifest.Waits
+		err := createByManifest(c, dc, manifest)
+		if err != nil {
+			return err
+		}
+
+		if waits == nil {
+			logger.Log.Info("no wait-for strategy is provided")
+			continue
+		}
+
+		for _, wait := range waits {
+			if strings.Contains(wait.Resource, "/") && wait.LabelSelector != "" {
+				return fmt.Errorf("when passing resource.group/resource.name in Resource, the labelSelector can not be set at the same time")
+			}
+
+			logger.Log.Infof("waiting for %+v", wait)

Review comment:
       It's too early to print this log?

##########
File path: internal/constant/kind.go
##########
@@ -18,8 +18,19 @@
 
 package constant
 
+import "time"
+
 const (
 	Kind                   = "kind"
 	KindCommand            = "kind"
 	KindClusterDefaultName = "kind"
+	E2EDefaultFile         = "e2e.yaml"
+	K8sClusterConfigFile   = "e2e-k8s.config"
+	DefaultWaitTimeout     = 600 * time.Second
+	AWeekWaitTimeout       = 7 * 24 * 60 * 60 * time.Second

Review comment:
       😂 this is way too long in a test framework, consider setting to half an hour

##########
File path: internal/components/setup/kind.go
##########
@@ -19,32 +19,83 @@
 package setup
 
 import (
+	"fmt"
+	"io/ioutil"
+	"os"
 	"strings"
+	"time"
+
+	"k8s.io/cli-runtime/pkg/genericclioptions"
+	ctlwait "k8s.io/kubectl/pkg/cmd/wait"
+
+	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/kubernetes"
+
+	"github.com/apache/skywalking-infra-e2e/internal/config"
+
+	apiv1 "k8s.io/api/admission/v1"
+
+	"github.com/apache/skywalking-infra-e2e/internal/util"
 
 	kind "sigs.k8s.io/kind/cmd/kind/app"
 	kindcmd "sigs.k8s.io/kind/pkg/cmd"
 
 	"github.com/apache/skywalking-infra-e2e/internal/constant"
 
-	"github.com/apache/skywalking-infra-e2e/internal/flags"
 	"github.com/apache/skywalking-infra-e2e/internal/logger"
 )
 
 var (
 	kindConfigFile string
+	kubeConfigPath string
 )
 
-func KindSetupInCommand() error {
-	kindConfigFile = flags.File
+// KindSetup sets up environment according to e2e.yaml.
+func KindSetup(e2eConfig *config.E2EConfig) error {
+	kindConfigFile = e2eConfig.Setup.File
+
+	timeout := e2eConfig.Setup.Timeout
+	var waitTimeout time.Duration
+	if timeout == 0 {
+		waitTimeout = constant.DefaultWaitTimeout
+	} else {
+		waitTimeout = time.Duration(timeout) * time.Second
+	}
+
+	logger.Log.Debugf("wait timeout is %d seconds", int(waitTimeout.Seconds()))
+
+	if kindConfigFile == "" {
+		return fmt.Errorf("no kind config file was provided")
+	}
+
+	manifests := e2eConfig.Setup.Manifests
+	// if no manifests was provided, then no need to create the cluster.
+	if manifests == nil {
+		logger.Log.Info("no manifests is provided")
+		return nil
+	}

Review comment:
       Move these to the top of the function

##########
File path: internal/config/globalConfig.go
##########
@@ -0,0 +1,69 @@
+//
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package config
+
+import (
+	"fmt"
+	"io/ioutil"
+
+	"github.com/apache/skywalking-infra-e2e/internal/logger"
+
+	"gopkg.in/yaml.v2"
+
+	"github.com/apache/skywalking-infra-e2e/internal/util"
+)
+
+// GlobalE2EConfig store E2EConfig which can be used globally.
+type GlobalE2EConfig struct {
+	Ready     bool
+	E2EConfig E2EConfig
+}
+
+var GlobalConfig GlobalE2EConfig
+
+func ReadGlobalConfigFile(configFilePath string) error {
+	if GlobalConfig.Ready {
+		logger.Log.Info("e2e config has been initialized")
+		return nil
+	}
+
+	e2eFile := configFilePath
+	if util.PathExist(e2eFile) {

Review comment:
       Return `fmt.Errorf("e2e config file %s not exist", e2eFile)` early `if !util.PathExist`, reducing the blocks

##########
File path: internal/flags/setup.go
##########
@@ -17,6 +17,3 @@
 //
 
 package flags
-
-var Env string
-var File string

Review comment:
       We don't need this file




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org