From d44049da9f49b8bf98af60d820612517f1b22d4b Mon Sep 17 00:00:00 2001
From: Jun Gong <jungong@tencent.com>
Date: Wed, 20 Mar 2019 09:17:44 +0800
Subject: [PATCH] Change run option SchedulePeriod's type to make it clear

---
 cmd/kube-batch/app/options/options.go      | 20 +++++----
 cmd/kube-batch/app/options/options_test.go | 48 ++++++++++++++++++++++
 pkg/scheduler/scheduler.go                 |  5 +--
 3 files changed, 62 insertions(+), 11 deletions(-)
 create mode 100644 cmd/kube-batch/app/options/options_test.go

diff --git a/cmd/kube-batch/app/options/options.go b/cmd/kube-batch/app/options/options.go
index 6fc4d636c..dd7c24fff 100644
--- a/cmd/kube-batch/app/options/options.go
+++ b/cmd/kube-batch/app/options/options.go
@@ -23,13 +23,20 @@ import (
 	"github.com/spf13/pflag"
 )
 
+const (
+	defaultSchedulerName   = "kube-batch"
+	defaultSchedulerPeriod = time.Second
+	defaultQueue           = "default"
+	defaultListenAddress   = ":8080"
+)
+
 // ServerOption is the main context object for the controller manager.
 type ServerOption struct {
 	Master               string
 	Kubeconfig           string
 	SchedulerName        string
 	SchedulerConf        string
-	SchedulePeriod       string
+	SchedulePeriod       time.Duration
 	EnableLeaderElection bool
 	LockObjectNamespace  string
 	DefaultQueue         string
@@ -48,25 +55,22 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.Master, "master", s.Master, "The address of the Kubernetes API server (overrides any value in kubeconfig)")
 	fs.StringVar(&s.Kubeconfig, "kubeconfig", s.Kubeconfig, "Path to kubeconfig file with authorization and master location information")
 	// kube-batch will ignore pods with scheduler names other than specified with the option
-	fs.StringVar(&s.SchedulerName, "scheduler-name", "kube-batch", "kube-batch will handle pods with the scheduler-name")
+	fs.StringVar(&s.SchedulerName, "scheduler-name", defaultSchedulerName, "kube-batch will handle pods with the scheduler-name")
 	fs.StringVar(&s.SchedulerConf, "scheduler-conf", "", "The absolute path of scheduler configuration file")
-	fs.StringVar(&s.SchedulePeriod, "schedule-period", "1s", "The period between each scheduling cycle")
-	fs.StringVar(&s.DefaultQueue, "default-queue", "default", "The default queue name of the job")
+	fs.DurationVar(&s.SchedulePeriod, "schedule-period", defaultSchedulerPeriod, "The period between each scheduling cycle")
+	fs.StringVar(&s.DefaultQueue, "default-queue", defaultQueue, "The default queue name of the job")
 	fs.BoolVar(&s.EnableLeaderElection, "leader-elect", s.EnableLeaderElection,
 		"Start a leader election client and gain leadership before "+
 			"executing the main loop. Enable this when running replicated kube-batch for high availability")
 	fs.BoolVar(&s.PrintVersion, "version", false, "Show version and quit")
 	fs.StringVar(&s.LockObjectNamespace, "lock-object-namespace", s.LockObjectNamespace, "Define the namespace of the lock object")
-	fs.StringVar(&s.ListenAddress, "listen-address", ":8080", "The address to listen on for HTTP requests.")
+	fs.StringVar(&s.ListenAddress, "listen-address", defaultListenAddress, "The address to listen on for HTTP requests.")
 }
 
 func (s *ServerOption) CheckOptionOrDie() error {
 	if s.EnableLeaderElection && s.LockObjectNamespace == "" {
 		return fmt.Errorf("lock-object-namespace must not be nil when LeaderElection is enabled")
 	}
-	if _, err := time.ParseDuration(s.SchedulePeriod); err != nil {
-		return fmt.Errorf("failed to parse --schedule-period: %v", err)
-	}
 
 	return nil
 }
diff --git a/cmd/kube-batch/app/options/options_test.go b/cmd/kube-batch/app/options/options_test.go
new file mode 100644
index 000000000..d634e6288
--- /dev/null
+++ b/cmd/kube-batch/app/options/options_test.go
@@ -0,0 +1,48 @@
+/*
+Copyright 2019 The Kubernetes Authors.
+
+Licensed 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 options
+
+import (
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/spf13/pflag"
+)
+
+func TestAddFlags(t *testing.T) {
+	fs := pflag.NewFlagSet("addflagstest", pflag.ContinueOnError)
+	s := NewServerOption()
+	s.AddFlags(fs)
+
+	args := []string{
+		"--schedule-period=5m",
+	}
+	fs.Parse(args)
+
+	// This is a snapshot of expected options parsed by args.
+	expected := &ServerOption{
+		SchedulerName:  defaultSchedulerName,
+		SchedulePeriod: 5 * time.Minute,
+		DefaultQueue:   defaultQueue,
+		ListenAddress:  defaultListenAddress,
+	}
+
+	if !reflect.DeepEqual(expected, s) {
+		t.Errorf("Got different run options than expected.\nGot: %+v\nExpected: %+v\n", s, expected)
+	}
+}
diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go
index 99f4da0d2..3e587370d 100644
--- a/pkg/scheduler/scheduler.go
+++ b/pkg/scheduler/scheduler.go
@@ -43,15 +43,14 @@ func NewScheduler(
 	config *rest.Config,
 	schedulerName string,
 	conf string,
-	period string,
+	period time.Duration,
 	defaultQueue string,
 ) (*Scheduler, error) {
-	sp, _ := time.ParseDuration(period)
 	scheduler := &Scheduler{
 		config:         config,
 		schedulerConf:  conf,
 		cache:          schedcache.New(config, schedulerName, defaultQueue),
-		schedulePeriod: sp,
+		schedulePeriod: period,
 	}
 
 	return scheduler, nil