Remove deprecated parameters (#489)

Remove support for old job command parameters

Deprecated long ago, Job params: command, shell and environment_variables are now gone.

This could be breaking change for installations still using those params

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Unreleased
 
 - Add DynamoDB support
+- Breaking: Remove support and deprecation message for old command, environment_variables and shell parameters
 
 ## 1.0.2
 

builtin/bins/dkron-executor-shell/shell_test.go

@@ -0,0 +1,38 @@
+package main
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_buildCmd(t *testing.T) {
+
+	// test shell command
+	cmd, err := buildCmd("echo 'test1' && echo 'success'", true, []string{}, "")
+	assert.NoError(t, err)
+	out, err := cmd.CombinedOutput()
+	assert.NoError(t, err)
+	assert.Equal(t, "test1\nsuccess\n", string(out))
+
+	// test not shell command
+	cmd, err = buildCmd("date && echo 'success'", false, []string{}, "")
+	assert.NoError(t, err)
+	out, err = cmd.CombinedOutput()
+	assert.Error(t, err)
+}
+
+func Test_buildCmdWithCustomEnvironmentVariables(t *testing.T) {
+	defer func() {
+		os.Setenv("Foo", "")
+	}()
+	os.Setenv("Foo", "Bar")
+
+	cmd, err := buildCmd("echo $Foo && echo $He", true, []string{"Foo=Toto", "He=Ho"}, "")
+	assert.NoError(t, err)
+	out, err := cmd.CombinedOutput()
+	assert.NoError(t, err)
+	assert.Equal(t, "Toto\nHo\n", string(out))
+
+}

dkron/agent.go

@@ -463,15 +463,15 @@ func (a *Agent) eventLoop() {
 						log.WithError(err).Error("agent: Error on rpc.GetJob call")
 						continue
 					}
-					log.WithField("command", job.Command).Debug("agent: GetJob by RPC")
+					log.WithField("job", job.Name).Debug("agent: GetJob by RPC")
 
 					ex := rqp.Execution
 					ex.StartedAt = time.Now()
 					ex.NodeName = a.config.NodeName
 
 					go func() {
 						if err := a.invokeJob(job, ex); err != nil {
-							log.WithError(err).Error("agent: Error invoking job command")
+							log.WithError(err).Error("agent: Error invoking job")
 						}
 					}()
 

dkron/agent_test.go

@@ -12,7 +12,7 @@ import (
 )
 
 var (
-	logLevel = "debug"
+	logLevel = "error"
 	etcdAddr = getEnvWithDefault()
 )
 

dkron/api.go

@@ -170,7 +170,7 @@ func (h *HTTPTransport) jobCreateOrUpdateHandler(c *gin.Context) {
 	h.agent.SchedulerRestart()
 
 	c.Header("Location", fmt.Sprintf("%s/%s", c.Request.RequestURI, job.Name))
-	renderJSON(c, http.StatusCreated, job)
+	renderJSON(c, http.StatusCreated, &job)
 }
 
 func (h *HTTPTransport) jobDeleteHandler(c *gin.Context) {
@@ -237,9 +237,6 @@ func (h *HTTPTransport) leaderHandler(c *gin.Context) {
 }
 
 func (h *HTTPTransport) leaveHandler(c *gin.Context) {
-	if c.Request.Method == http.MethodGet {
-		log.Warn("/leave GET is deprecated and will be removed, use POST")
-	}
 	if err := h.agent.serf.Leave(); err != nil {
 		renderJSON(c, http.StatusOK, h.agent.listServers())
 	}

dkron/api_test.go

@@ -44,7 +44,7 @@ func setupAPITest(t *testing.T) (a *Agent) {
 func TestAPIJobCreateUpdate(t *testing.T) {
 	a := setupAPITest(t)
 
-	jsonStr := []byte(`{"name": "test_job", "schedule": "@every 1m", "command": "date", "owner": "mec", "owner_email": "foo@bar.com", "disabled": true}`)
+	jsonStr := []byte(`{"name": "test_job", "schedule": "@every 1m", "executor": "shell", "executor_config": {"command": "date"}, "owner": "mec", "owner_email": "foo@bar.com", "disabled": true}`)
 
 	resp, err := http.Post("http://localhost:8090/v1/jobs", "encoding/json", bytes.NewBuffer(jsonStr))
 	if err != nil {
@@ -59,7 +59,7 @@ func TestAPIJobCreateUpdate(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	jsonStr1 := []byte(`{"name": "test_job", "schedule": "@every 1m", "command": "test", "disabled": false}`)
+	jsonStr1 := []byte(`{"name": "test_job", "schedule": "@every 1m", "executor": "shell", "executor_config": {"command": "test"}, "disabled": false}`)
 	resp, err = http.Post("http://localhost:8090/v1/jobs", "encoding/json", bytes.NewBuffer(jsonStr1))
 	if err != nil {
 		t.Fatal(err)
@@ -75,8 +75,8 @@ func TestAPIJobCreateUpdate(t *testing.T) {
 
 	assert.Equal(t, origJob.Name, overwriteJob.Name)
 	assert.False(t, overwriteJob.Disabled)
-	assert.NotEqual(t, origJob.Command, overwriteJob.Command)
-	assert.Equal(t, "test", overwriteJob.Command)
+	assert.NotEqual(t, origJob.ExecutorConfig["command"], overwriteJob.ExecutorConfig["command"])
+	assert.Equal(t, "test", overwriteJob.ExecutorConfig["command"])
 
 	// Send a shutdown request
 	a.Stop()

dkron/grpc.go

@@ -72,9 +72,6 @@ func (grpcs *GRPCServer) GetJob(ctx context.Context, getJobReq *proto.GetJobRequ
 
 	// Copy the data structure
 	gjr.Name = j.Name
-	gjr.Shell = j.Shell
-	gjr.EnvironmentVariables = j.EnvironmentVariables
-	gjr.Command = j.Command
 	gjr.Executor = j.Executor
 	gjr.ExecutorConfig = j.ExecutorConfig
 

dkron/grpc_test.go

@@ -38,10 +38,11 @@ func TestGRPCExecutionDone(t *testing.T) {
 	time.Sleep(2 * time.Second)
 
 	testJob := &Job{
-		Name:     "test",
-		Schedule: "@every 1m",
-		Command:  "/bin/false",
-		Disabled: true,
+		Name:           "test",
+		Schedule:       "@every 1m",
+		Executor:       "shell",
+		ExecutorConfig: map[string]string{"command": "/bin/false"},
+		Disabled:       true,
 	}
 
 	if err := store.SetJob(testJob, true); err != nil {

dkron/invoke.go

@@ -1,17 +1,12 @@
 package dkron
 
 import (
+	"errors"
 	"math/rand"
-	"os"
-	"os/exec"
-	"runtime"
-	"strconv"
-	"strings"
 	"time"
 
 	"github.com/armon/circbuf"
 	"github.com/hashicorp/serf/serf"
-	"github.com/mattn/go-shellwords"
 )
 
 const (
@@ -32,15 +27,7 @@ func (a *Agent) invokeJob(job *Job, execution *Execution) error {
 	jex := job.Executor
 	exc := job.ExecutorConfig
 	if jex == "" {
-		jex = "shell"
-		exc = map[string]string{
-			"command": job.Command,
-			"shell":   strconv.FormatBool(job.Shell),
-			"env":     strings.Join(job.EnvironmentVariables, ","),
-		}
-		log.Warning("invoke: Deprecation waring! fields command, " +
-			"shell and environment_variables params are deprecated and will be removed in future versions. " +
-			"Consider migrating the job definition to the shell executor plugin")
+		return errors.New("invoke: No executor defined, nothing to do")
 	}
 
 	// Check if executor is exists
@@ -93,30 +80,3 @@ func (a *Agent) getServerRPCAddresFromTags() (string, error) {
 	}
 	return "", ErrNoRPCAddress
 }
-
-// Determine the shell invocation based on OS
-func buildCmd(job *Job) (cmd *exec.Cmd) {
-	var shell, flag string
-
-	if job.Shell {
-		if runtime.GOOS == windows {
-			shell = "cmd"
-			flag = "/C"
-		} else {
-			shell = "/bin/sh"
-			flag = "-c"
-		}
-		cmd = exec.Command(shell, flag, job.Command)
-	} else {
-		args, err := shellwords.Parse(job.Command)
-		if err != nil {
-			log.WithError(err).Fatal("invoke: Error parsing command arguments")
-		}
-		cmd = exec.Command(args[0], args[1:]...)
-	}
-	if job.EnvironmentVariables != nil {
-		cmd.Env = append(os.Environ(), job.EnvironmentVariables...)
-	}
-
-	return
-}

dkron/invoke_test.go

@@ -1,49 +1 @@
 package dkron
-
-import (
-	"os"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func Test_buildCmd(t *testing.T) {
-
-	// test shell command
-	testJob1 := &Job{
-		Command: "echo 'test1' && echo 'success'",
-		Shell:   true,
-	}
-
-	cmd := buildCmd(testJob1)
-	out, err := cmd.CombinedOutput()
-	assert.NoError(t, err)
-	assert.Equal(t, "test1\nsuccess\n", string(out))
-
-	// test not shell command
-	testJob2 := &Job{
-		Command: "date && echo 'success'",
-		Shell:   false,
-	}
-	cmd = buildCmd(testJob2)
-	out, err = cmd.CombinedOutput()
-	assert.Error(t, err)
-}
-
-func Test_buildCmdWithCustomEnvironmentVariables(t *testing.T) {
-	defer func() {
-		os.Setenv("Foo", "")
-	}()
-	os.Setenv("Foo", "Bar")
-	testJob := &Job{
-		Command:              "echo $Foo && echo $He",
-		EnvironmentVariables: []string{"Foo=Toto", "He=Ho"},
-		Shell:                true,
-	}
-
-	cmd := buildCmd(testJob)
-	out, err := cmd.CombinedOutput()
-	assert.NoError(t, err)
-	assert.Equal(t, "Toto\nHo\n", string(out))
-
-}

dkron/job.go

@@ -55,15 +55,6 @@ type Job struct {
 	// Cron expression for the job. When to run the job.
 	Schedule string `json:"schedule"`
 
-	// Use shell to run the command.
-	Shell bool `json:"shell"`
-
-	// Command to run. Must be a shell command to execute.
-	Command string `json:"command"`
-
-	// Extra environment variable to give to the command to execute.
-	EnvironmentVariables []string `json:"environment_variables"`
-
 	// Owner of the job.
 	Owner string `json:"owner"`
 
@@ -122,25 +113,22 @@ type Job struct {
 
 func NewJobFromProto(in *proto.GetJobResponse) *Job {
 	return &Job{
-		Name:                 in.Name,
-		Timezone:             in.Timezone,
-		Schedule:             in.Schedule,
-		Shell:                in.Shell,
-		Command:              in.Command,
-		EnvironmentVariables: in.EnvironmentVariables,
-		Owner:                in.Owner,
-		OwnerEmail:           in.OwnerEmail,
-		SuccessCount:         int(in.SuccessCount),
-		ErrorCount:           int(in.ErrorCount),
-		Disabled:             in.Disabled,
-		Tags:                 in.Tags,
-		Retries:              uint(in.Retries),
-		DependentJobs:        in.DependentJobs,
-		ParentJob:            in.ParentJob,
-		Concurrency:          in.Concurrency,
-		Executor:             in.Executor,
-		ExecutorConfig:       in.ExecutorConfig,
-		Status:               in.Status,
+		Name:           in.Name,
+		Timezone:       in.Timezone,
+		Schedule:       in.Schedule,
+		Owner:          in.Owner,
+		OwnerEmail:     in.OwnerEmail,
+		SuccessCount:   int(in.SuccessCount),
+		ErrorCount:     int(in.ErrorCount),
+		Disabled:       in.Disabled,
+		Tags:           in.Tags,
+		Retries:        uint(in.Retries),
+		DependentJobs:  in.DependentJobs,
+		ParentJob:      in.ParentJob,
+		Concurrency:    in.Concurrency,
+		Executor:       in.Executor,
+		ExecutorConfig: in.ExecutorConfig,
+		Status:         in.Status,
 	}
 }
 

dkron/job_test.go

@@ -21,19 +21,21 @@ func TestJobGetParent(t *testing.T) {
 	}
 
 	parentTestJob := &Job{
-		Name:     "parent_test",
-		Command:  "/bin/false",
-		Schedule: "@every 2s",
+		Name:           "parent_test",
+		Executor:       "shell",
+		ExecutorConfig: map[string]string{"command": "/bin/false"},
+		Schedule:       "@every 2s",
 	}
 
 	if err := store.SetJob(parentTestJob, true); err != nil {
 		t.Fatalf("error creating job: %s", err)
 	}
 
 	dependentTestJob := &Job{
-		Name:      "dependent_test",
-		Command:   "/bin/false",
-		ParentJob: "parent_test",
+		Name:           "dependent_test",
+		Executor:       "shell",
+		ExecutorConfig: map[string]string{"command": "/bin/false"},
+		ParentJob:      "parent_test",
 	}
 
 	err = store.SetJob(dependentTestJob, true)

dkron/scheduler_test.go

@@ -13,12 +13,12 @@ func TestSchedule(t *testing.T) {
 	assert.False(t, sched.Started)
 
 	testJob1 := &Job{
-		Name:       "cron_job",
-		Schedule:   "@every 2s",
-		Command:    "echo 'test1'",
-		Owner:      "John Dough",
-		OwnerEmail: "foo@bar.com",
-		Shell:      true,
+		Name:           "cron_job",
+		Schedule:       "@every 2s",
+		Executor:       "shell",
+		ExecutorConfig: map[string]string{"command": "echo 'test1'", "shell": "true"},
+		Owner:          "John Dough",
+		OwnerEmail:     "foo@bar.com",
 	}
 	sched.Start([]*Job{testJob1})
 
@@ -28,12 +28,12 @@ func TestSchedule(t *testing.T) {
 	assert.Equal(t, now.Add(time.Second*2), sched.GetEntry(testJob1).Next)
 
 	testJob2 := &Job{
-		Name:       "cron_job",
-		Schedule:   "@every 5s",
-		Command:    "echo 'test2'",
-		Owner:      "John Dough",
-		OwnerEmail: "foo@bar.com",
-		Shell:      true,
+		Name:           "cron_job",
+		Schedule:       "@every 5s",
+		Executor:       "shell",
+		ExecutorConfig: map[string]string{"command": "echo 'test2'", "shell": "true"},
+		Owner:          "John Dough",
+		OwnerEmail:     "foo@bar.com",
 	}
 	sched.Restart([]*Job{testJob2})