ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/nomad
File: nomad/encrypter_test.go (Go)

Code before:
	// Launch the concurrent calls to AddWrappedKey and wait till they have all
	// triggered and responded before moving on. The timeout ensures this test
	// won't deadlock or hang indefinitely.
	var wg sync.WaitGroup
	wg.Add(concurrentNum)

	for i := 0; i < concurrentNum; i++ {
		go func() {
			<-startCh
			respCh <- encrypter.AddWrappedKey(timeoutCtx, wrappedKey)
			wg.Done()
		}()
	}

	close(startCh)
	wg.Wait()

	// Gather the responses and ensure the encrypter state is as we expect.
	var respNum int

	for resp := range respCh {
		must.NoError(t, resp)
		if respNum++; respNum == concurrentNum {
			break
		}
	}

	must.NoError(t, encrypter.IsReady(timeoutCtx))
	must.MapEmpty(t, encrypter.decryptTasks)
	must.MapLen(t, 1, encrypter.keyring)
	must.MapContainsKey(t, encrypter.keyring, key.Meta.KeyID)
}

func TestEncrypter_decryptWrappedKeyTask_successful(t *testing.T) {
	ci.Parallel(t)

	srv := &Server{
		logger: testlog.HCLogger(t),
		config: &Config{},
	}

	key, err := structs.NewUnwrappedRootKey(structs.EncryptionAlgorithmAES256GCM)
	must.NoError(t, err)

	encrypter, err := NewEncrypter(srv, t.TempDir())
	must.NoError(t, err)

	wrappedKey, err := encrypter.encryptDEK(key, &structs.KEKProviderConfig{})
	must.NotNil(t, wrappedKey)
	must.NoError(t, err)


Review comment (suggestion):
Wouldn't this hang if one of the `AddWrappedKey` calls hangs? Maybe this needs to be in a loop + `select` with the context? Ex. something like:

```suggestion
	for {
		select {
			case resp <-respCh:
				must.NoError(t, resp)
				if respNum++; respNum == concurrentNum {
					break
				}
			case <-timeoutCtx.Done():
				must.NoError(t, timeoutCtx.Err())
		}	
	}
```

Code after:
	// Launch the concurrent calls to AddWrappedKey and wait till they have all
	// triggered and responded before moving on. The timeout ensures this test
	// won't deadlock or hang indefinitely.
	var wg sync.WaitGroup
	wg.Add(concurrentNum)

	for i := 0; i < concurrentNum; i++ {
		go func() {
			<-startCh
			respCh <- encrypter.AddWrappedKey(timeoutCtx, wrappedKey)
			wg.Done()
		}()
	}

	close(startCh)
	wg.Wait()

	// Gather the responses and ensure the encrypter state is as we expect.
	var respNum int

	for {
		select {
		case resp := <-respCh:
			must.NoError(t, resp)
			if respNum++; respNum == concurrentNum {
				must.NoError(t, encrypter.IsReady(timeoutCtx))
				must.MapEmpty(t, encrypter.decryptTasks)
				must.MapLen(t, 1, encrypter.keyring)
				must.MapContainsKey(t, encrypter.keyring, key.Meta.KeyID)
				return
			}
		case <-timeoutCtx.Done():
			must.NoError(t, timeoutCtx.Err())
		}
	}
}

func TestEncrypter_decryptWrappedKeyTask_successful(t *testing.T) {
	ci.Parallel(t)

	srv := &Server{
		logger: testlog.HCLogger(t),
		config: &Config{},
	}

	key, err := structs.NewUnwrappedRootKey(structs.EncryptionAlgorithmAES256GCM)
	must.NoError(t, err)

	encrypter, err := NewEncrypter(srv, t.TempDir())
	must.NoError(t, err)
Repository: hashicorp/nomad
File: nomad/state/state_store_test.go (Go)

Code before:
	require.Equal(t, int64(0), summary.Children.Dead)

	require.False(t, watchFired(ws))
}

func TestStateStore_UpsertAlloc_NextAllocation(t *testing.T) {
	ci.Parallel(t)

	state := testStateStore(t)

	alloc1 := mock.Alloc()
	alloc2 := mock.Alloc()
	alloc2.PreviousAllocation = alloc1.ID

	err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc1})
	require.NoError(t, err)

	err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1001, []*structs.Allocation{alloc2})
	require.NoError(t, err)

	// upsertings alloc2 should update alloc1
	actual, err := state.AllocByID(nil, alloc1.ID)
	must.Eq(t, actual.NextAllocation, alloc2.ID)

	err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1002, []*structs.Allocation{alloc1})
	require.NoError(t, err)

	// upserting alloc1 again should carry over the NextAllocation
	must.Eq(t, alloc1.NextAllocation, alloc2.ID)
}

func TestStateStore_UpdateAlloc_Alloc(t *testing.T) {
	ci.Parallel(t)

	state := testStateStore(t)
	alloc := mock.Alloc()

	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, nil, alloc.Job); err != nil {
		t.Fatalf("err: %v", err)
	}

	err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc})
	if err != nil {
		t.Fatalf("err: %v", err)
	}

	ws := memdb.NewWatchSet()
	summary, err := state.JobSummaryByID(ws, alloc.Namespace, alloc.JobID)
	if err != nil {
		t.Fatalf("err: %v", err)
	}

Review comment (suggestion):
Let's not add new tests with `testify` instead of `shoenig/test`:

```suggestion
	must.NoError(t, err)
```

(there's a few of these here)

Code after:
	require.Equal(t, int64(0), summary.Children.Dead)

	require.False(t, watchFired(ws))
}

func TestStateStore_UpsertAlloc_NextAllocation(t *testing.T) {
	ci.Parallel(t)

	state := testStateStore(t)

	alloc1 := mock.Alloc()
	alloc2 := mock.Alloc()
	alloc2.PreviousAllocation = alloc1.ID

	err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc1, alloc2})
	must.NoError(t, err)

	// alloc1 should have the correct NextAllocation
	actual, err := state.AllocByID(nil, alloc1.ID)
	must.Eq(t, actual.NextAllocation, alloc2.ID)

	err = state.UpsertAllocs(structs.MsgTypeTestSetup, 1001, []*structs.Allocation{alloc2, alloc1})
	must.NoError(t, err)

	// upsert in a different order, alloc1 should still have the correct NextAllocation
	actual, err = state.AllocByID(nil, alloc1.ID)
	must.NoError(t, err)
	must.Eq(t, actual.NextAllocation, alloc2.ID)
}

func TestStateStore_UpdateAlloc_Alloc(t *testing.T) {
	ci.Parallel(t)

	state := testStateStore(t)
	alloc := mock.Alloc()

	if err := state.UpsertJob(structs.MsgTypeTestSetup, 999, nil, alloc.Job); err != nil {
		t.Fatalf("err: %v", err)
	}

	err := state.UpsertAllocs(structs.MsgTypeTestSetup, 1000, []*structs.Allocation{alloc})
	if err != nil {
		t.Fatalf("err: %v", err)
	}

	ws := memdb.NewWatchSet()
	summary, err := state.JobSummaryByID(ws, alloc.Namespace, alloc.JobID)
	if err != nil {
		t.Fatalf("err: %v", err)
	}
	tgSummary := summary.Summary["web"]
Repository: hashicorp/nomad
File: scheduler/scheduler_system.go (Go)

Code before:
	if !s.job.Stopped() && s.job.Update.Rolling() {
		limit = s.job.Update.MaxParallel
	}

	// Treat non in-place updates as an eviction and new placement.
	s.limitReached = evictAndPlace(s.ctx, diff, diff.update, allocUpdating, &limit)

	// Nothing remaining to do if placement is not required
	if len(diff.place) == 0 {
		if !s.job.Stopped() {
			for _, tg := range s.job.TaskGroups {
				s.queuedAllocs[tg.Name] = 0
			}
		}
		return nil
	}

	// Record the number of allocations that needs to be placed per Task Group
	for _, allocTuple := range diff.place {
		s.queuedAllocs[allocTuple.TaskGroup.Name] += 1
	}

	for _, ignoredAlloc := range diff.ignore {
		if !allocExistsForTaskGroup[ignoredAlloc.TaskGroup.Name] {
			allocExistsForTaskGroup[ignoredAlloc.TaskGroup.Name] = true
		}
	}

	// Compute the placements
	return s.computePlacements(diff.place, allocExistsForTaskGroup)
}

func mergeNodeFiltered(acc, curr *structs.AllocMetric) *structs.AllocMetric {
	if acc == nil {
		return curr.Copy()
	}

	acc.NodesEvaluated += curr.NodesEvaluated
	acc.NodesFiltered += curr.NodesFiltered

	if acc.ClassFiltered == nil {
		acc.ClassFiltered = make(map[string]int)
	}
	for k, v := range curr.ClassFiltered {
		acc.ClassFiltered[k] += v
	}
	if acc.ConstraintFiltered == nil {
		acc.ConstraintFiltered = make(map[string]int)
	}
	for k, v := range curr.ConstraintFiltered {
		acc.ConstraintFiltered[k] += v

Review comment (suggestion):
Same as above.

```suggestion
		allocExistsForTaskGroup[ignoredAlloc.TaskGroup.Name]  = true
```

Code after:
	}

	// Treat non in-place updates as an eviction and new placement.
	s.limitReached = evictAndPlace(s.ctx, diff, diff.update, allocUpdating, &limit)

	// Nothing remaining to do if placement is not required
	if len(diff.place) == 0 {
		if !s.job.Stopped() {
			for _, tg := range s.job.TaskGroups {
				s.queuedAllocs[tg.Name] = 0
			}
		}
		return nil
	}

	// Record the number of allocations that needs to be placed per Task Group
	for _, allocTuple := range diff.place {
		s.queuedAllocs[allocTuple.TaskGroup.Name] += 1
	}

	for _, ignoredAlloc := range diff.ignore {
		allocExistsForTaskGroup[ignoredAlloc.TaskGroup.Name] = true
	}

	// Compute the placements
	return s.computePlacements(diff.place, allocExistsForTaskGroup)
}

func mergeNodeFiltered(acc, curr *structs.AllocMetric) *structs.AllocMetric {
	if acc == nil {
		return curr.Copy()
	}

	acc.NodesEvaluated += curr.NodesEvaluated
	acc.NodesFiltered += curr.NodesFiltered

	if acc.ClassFiltered == nil {
		acc.ClassFiltered = make(map[string]int)
	}
	for k, v := range curr.ClassFiltered {
		acc.ClassFiltered[k] += v
	}
	if acc.ConstraintFiltered == nil {
		acc.ConstraintFiltered = make(map[string]int)
	}
	for k, v := range curr.ConstraintFiltered {
		acc.ConstraintFiltered[k] += v
	}
	acc.AllocationTime += curr.AllocationTime
	return acc
}
Repository: hashicorp/nomad
File: drivers/shared/executor/executor_unix.go (Go)

Code before:
func setCmdUser(cmd *exec.Cmd, userid string) error {
	u, err := users.Lookup(userid)
	if err != nil {
		return fmt.Errorf("failed to identify user %v: %v", userid, err)
	}

	// Get the groups the user is a part of
	gidStrings, err := u.GroupIds()
	if err != nil {
		return fmt.Errorf("unable to lookup user's group membership: %v", err)
	}

	gids := make([]uint32, len(gidStrings))
	for _, gidString := range gidStrings {
		u, err := strconv.ParseUint(gidString, 10, 32)
		if err != nil {
			return fmt.Errorf("unable to convert user's group to uint32 %s: %v", gidString, err)
		}

		gids = append(gids, uint32(u))
	}

	// Convert the uid and gid
	uid, err := strconv.ParseUint(u.Uid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert userid to uint32: %s", err)
	}
	gid, err := strconv.ParseUint(u.Gid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert groupid to uint32: %s", err)
	}

	// Set the command to run as that user and group.
	if cmd.SysProcAttr == nil {
		cmd.SysProcAttr = &syscall.SysProcAttr{}
	}
	if cmd.SysProcAttr.Credential == nil {
		cmd.SysProcAttr.Credential = &syscall.Credential{}
	}
	cmd.SysProcAttr.Credential.Uid = uint32(uid)
	cmd.SysProcAttr.Credential.Gid = uint32(gid)
	cmd.SysProcAttr.Credential.Groups = gids

	// Override HOME and USER environment variables
	cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", u.Username))
	cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", u.HomeDir))

	return nil
}


Review comment (suggestion):
```suggestion
		return fmt.Errorf("unable to convert userid to uint32: %w", err)
```

(typical of the errors here)

Code after:
func setCmdUser(cmd *exec.Cmd, userid string) error {
	u, err := users.Lookup(userid)
	if err != nil {
		return fmt.Errorf("failed to identify user %v: %v", userid, err)
	}

	// Get the groups the user is a part of
	gidStrings, err := u.GroupIds()
	if err != nil {
		return fmt.Errorf("unable to lookup user's group membership: %v", err)
	}

	gids := make([]uint32, len(gidStrings))
	for _, gidString := range gidStrings {
		u, err := strconv.ParseUint(gidString, 10, 32)
		if err != nil {
			return fmt.Errorf("unable to convert user's group to uint32 %s: %v", gidString, err)
		}

		gids = append(gids, uint32(u))
	}

	// Convert the uid and gid
	uid, err := strconv.ParseUint(u.Uid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert userid to uint32: %w", err)
	}
	gid, err := strconv.ParseUint(u.Gid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert groupid to uint32: %s", err)
	}

	// Set the command to run as that user and group.
	if cmd.SysProcAttr == nil {
		cmd.SysProcAttr = &syscall.SysProcAttr{}
	}
	if cmd.SysProcAttr.Credential == nil {
		cmd.SysProcAttr.Credential = &syscall.Credential{}
	}
	cmd.SysProcAttr.Credential.Uid = uint32(uid)
	cmd.SysProcAttr.Credential.Gid = uint32(gid)
	cmd.SysProcAttr.Credential.Groups = gids

	// Override HOME and USER environment variables
	cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", u.Username))
	cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", u.HomeDir))

	return nil
}
Repository: hashicorp/nomad
File: drivers/shared/executor/executor_linux_test.go (Go)

Code before:

	ps, err := executor.Launch(execCmd)
	must.NoError(t, err)
	must.NonZero(t, ps.Pid)

	state, err := executor.Wait(context.Background())
	must.NoError(t, err)
	must.Zero(t, state.ExitCode)

	output := strings.TrimSpace(testExecCmd.stdout.String())
	must.Eq(t, output, workDir)
}

// TestExecutor_UserEnv tests that the USER environment variable is set
// correctly if user is set. We're not testing HOME because that could get
// tricky on GHA runners.
func TestExecutor_UserEnv(t *testing.T) {
	t.Parallel()
	testutil.ExecCompatible(t)

	testExecCmd := testExecutorCommandWithChroot(t)
	execCmd, allocDir := testExecCmd.command, testExecCmd.allocDir
	execCmd.Cmd = "/bin/bash"
	execCmd.Args = []string{"-c", "echo $USER"}
	execCmd.User = "runner"
	execCmd.ResourceLimits = true
	defer allocDir.Destroy()

	executor := NewExecutorWithIsolation(testlog.HCLogger(t), compute)
	defer executor.Shutdown("SIGKILL", 0)

	ps, err := executor.Launch(execCmd)
	must.NoError(t, err)
	must.NonZero(t, ps.Pid)

	state, err := executor.Wait(context.Background())
	must.NoError(t, err)
	must.Zero(t, state.ExitCode)

	_, ok := executor.(*LibcontainerExecutor)
	must.True(t, ok)

	output := strings.TrimSpace(testExecCmd.stdout.String())
	must.Eq(t, output, "runner")
}

func TestExecCommand_getCgroupOr_off(t *testing.T) {
	ci.Parallel(t)

	if cgroupslib.GetMode() != cgroupslib.OFF {
		t.Skip("test only runs with no cgroups")

Review comment (bug):
This test is always going to fail anywhere _except_ GitHub runners, right? Is there a `testutil` flag we can set to skip it?

Code after:

	ps, err := executor.Launch(execCmd)
	must.NoError(t, err)
	must.NonZero(t, ps.Pid)

	state, err := executor.Wait(context.Background())
	must.NoError(t, err)
	must.Zero(t, state.ExitCode)

	output := strings.TrimSpace(testExecCmd.stdout.String())
	must.Eq(t, output, workDir)
}

// TestExecutor_UserEnv tests that the USER environment variable is set
// correctly if user is set. We're not testing HOME because that could get
// tricky on GHA runners.
func TestExecutor_UserEnv(t *testing.T) {
	t.Parallel()
	testutil.RequireCILinux(t)
	testutil.ExecCompatible(t)

	testExecCmd := testExecutorCommandWithChroot(t)
	execCmd, allocDir := testExecCmd.command, testExecCmd.allocDir
	execCmd.Cmd = "/bin/bash"
	execCmd.Args = []string{"-c", "echo $USER"}
	execCmd.User = "runner"
	execCmd.ResourceLimits = true
	defer allocDir.Destroy()

	executor := NewExecutorWithIsolation(testlog.HCLogger(t), compute)
	defer executor.Shutdown("SIGKILL", 0)

	ps, err := executor.Launch(execCmd)
	must.NoError(t, err)
	must.NonZero(t, ps.Pid)

	state, err := executor.Wait(context.Background())
	must.NoError(t, err)
	must.Zero(t, state.ExitCode)

	_, ok := executor.(*LibcontainerExecutor)
	must.True(t, ok)

	output := strings.TrimSpace(testExecCmd.stdout.String())
	must.Eq(t, output, "runner")
}

func TestExecCommand_getCgroupOr_off(t *testing.T) {
	ci.Parallel(t)

	if cgroupslib.GetMode() != cgroupslib.OFF {
Repository: hashicorp/nomad
File: drivers/shared/executor/executor_unix.go (Go)

Code before:
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

//go:build unix

package executor

import (
	"fmt"
	"os"
	"syscall"
)

// configure new process group for child process
func (e *UniversalExecutor) setNewProcessGroup() error {
	if e.childCmd.SysProcAttr == nil {
		e.childCmd.SysProcAttr = &syscall.SysProcAttr{}
	}
	e.childCmd.SysProcAttr.Setpgid = true
	return nil
}

// SIGKILL the process group starting at process.Pid
func (e *UniversalExecutor) killProcessTree(process *os.Process) error {
	pid := process.Pid
	negative := -pid // tells unix to kill entire process group
	signal := syscall.SIGKILL

	// If new process group was created upon command execution
	// we can kill the whole process group now to cleanup any leftovers.
	if e.childCmd.SysProcAttr != nil && e.childCmd.SysProcAttr.Setpgid {
		e.logger.Trace("sending sigkill to process group", "pid", pid, "negative", negative, "signal", signal)
		if err := syscall.Kill(negative, signal); err != nil && err.Error() != noSuchProcessErr {
			return err
		}
		return nil
	}
	return process.Kill()
}

// Only send the process a shutdown signal (default INT), doesn't
// necessarily kill it.
func (e *UniversalExecutor) shutdownProcess(sig os.Signal, proc *os.Process) error {
	if sig == nil {
		sig = os.Interrupt
	}

	if err := proc.Signal(sig); err != nil && err.Error() != finishedErr {
		return fmt.Errorf("executor shutdown error: %v", err)
	}

	return nil
}


Review comment (suggestion):
There are could be a lot of variables missing. Also this could depend on the system where this is run.

@jbardin , @pkazmierczak 
I expect that something like `su - <username> -c "<command>"` will be run and let the system to decide which variables should be adjusted.

Code after:
	}

	// Convert the uid and gid
	uid, err := strconv.ParseUint(u.Uid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert userid to uint32: %w", err)
	}
	gid, err := strconv.ParseUint(u.Gid, 10, 32)
	if err != nil {
		return fmt.Errorf("unable to convert groupid to uint32: %s", err)
	}

	// Set the command to run as that user and group.
	if cmd.SysProcAttr == nil {
		cmd.SysProcAttr = &syscall.SysProcAttr{}
	}
	if cmd.SysProcAttr.Credential == nil {
		cmd.SysProcAttr.Credential = &syscall.Credential{}
	}
	cmd.SysProcAttr.Credential.Uid = uint32(uid)
	cmd.SysProcAttr.Credential.Gid = uint32(gid)
	cmd.SysProcAttr.Credential.Groups = gids

	// Override HOME and USER environment variables
	cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", u.Username))
	cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", u.HomeDir))

	return nil
}
Repository: hashicorp/nomad
File: nomad/structs/services.go (Go)

Code before:
	}
	nt := new(SidecarTask)
	*nt = *t
	nt.Env = maps.Clone(nt.Env)

	nt.Resources = nt.Resources.Copy()
	nt.LogConfig = nt.LogConfig.Copy()
	nt.Meta = maps.Clone(nt.Meta)

	if i, err := copystructure.Copy(nt.Config); err != nil {
		panic(err.Error())
	} else {
		nt.Config = i.(map[string]interface{})
	}

	if t.KillTimeout != nil {
		nt.KillTimeout = pointer.Of(*t.KillTimeout)
	}

	if t.ShutdownDelay != nil {
		nt.ShutdownDelay = pointer.Of(*t.ShutdownDelay)
	}

	nt.VolumeMounts = CopySliceVolumeMount(t.VolumeMounts)

	nt.Identities = CopySliceWorkloadIdentity(t.Identities)

	return nt
}

// MergeIntoTask merges the SidecarTask fields over the given task
func (t *SidecarTask) MergeIntoTask(task *Task) {
	if t.Name != "" {
		task.Name = t.Name
	}

	// If the driver changes then the driver config can be overwritten.
	// Otherwise we'll merge the driver config together
	if t.Driver != "" && t.Driver != task.Driver {
		task.Driver = t.Driver
		task.Config = t.Config
	} else {
		for k, v := range t.Config {
			task.Config[k] = v
		}
	}

	if t.User != "" {
		task.User = t.User
	}


Review comment (refactor):
We have a generic implementation we can use for anything that implements the `helper.Copy` interface, which WIs do

```suggestion
	nt.Identities = helper.CopySlice(t.Identities)
```

Code after:
	}
	nt := new(SidecarTask)
	*nt = *t
	nt.Env = maps.Clone(nt.Env)

	nt.Resources = nt.Resources.Copy()
	nt.LogConfig = nt.LogConfig.Copy()
	nt.Meta = maps.Clone(nt.Meta)

	if i, err := copystructure.Copy(nt.Config); err != nil {
		panic(err.Error())
	} else {
		nt.Config = i.(map[string]interface{})
	}

	if t.KillTimeout != nil {
		nt.KillTimeout = pointer.Of(*t.KillTimeout)
	}

	if t.ShutdownDelay != nil {
		nt.ShutdownDelay = pointer.Of(*t.ShutdownDelay)
	}

	nt.VolumeMounts = CopySliceVolumeMount(t.VolumeMounts)

	nt.Identities = helper.CopySlice(t.Identities)

	return nt
}

// MergeIntoTask merges the SidecarTask fields over the given task
func (t *SidecarTask) MergeIntoTask(task *Task) {
	if t.Name != "" {
		task.Name = t.Name
	}

	// If the driver changes then the driver config can be overwritten.
	// Otherwise we'll merge the driver config together
	if t.Driver != "" && t.Driver != task.Driver {
		task.Driver = t.Driver
		task.Config = t.Config
	} else {
		for k, v := range t.Config {
			task.Config[k] = v
		}
	}

	if t.User != "" {
		task.User = t.User
	}
Repository: hashicorp/nomad
File: command/job_start.go (Go)

Code before:
			if err == nil {
				now := time.Now().In(loc)
				next, err := job.Periodic.Next(now)
				if err != nil {
					c.Ui.Error(fmt.Sprintf("Error determining next launch time: %v", err))
				} else {
					c.Ui.Output(fmt.Sprintf("Approximate next launch time: %s (%s from now)",
						formatTime(next), formatTimeDifference(now, next, time.Second)))
				}
			}
		} else if !paramjob {
			c.Ui.Output("Evaluation ID: " + resp.EvalID)
		}

		return 0
	}

	mon := newMonitor(c.Ui, client, length)
	return mon.monitor(resp.EvalID)
}

func parseFromSubmission(sub *api.JobSubmission) (*api.Job, error) {
	var job *api.Job
	var err error

	switch sub.Format {
	case "hcl2":
		job, err = jobspec2.Parse("", strings.NewReader(sub.Source))
		if err != nil {
			return nil, fmt.Errorf("command: unable to parce job submission: %w", err)
		}

	case "json":
		err = json.Unmarshal([]byte(sub.Source), &job)
		if err != nil {
			return nil, fmt.Errorf("command: unable to parce job submission: %w", err)
		}
	}

	return job, nil
}


Review comment (suggestion):
If the previous version of the job was submitted directly via the HTTP API rather than from the CLI/UI, it won't have an associated submission object and `sub` will be nil and this function will panic. I see three possible ways to handle that:
1. Return an error that we just can't safely do `job start` for a job with scaling policies.
2. Enable all the policies unconditionally (maybe with a warning)?
3. Leave the policies disabled and provide a warning that we weren't able to safely determine whether we can re-enable policies?

I'm slightly in favor of option (3) just because it lets the user move forward without silent misbehavior, and only impacts users who are using the HTTP API directly.

Code after:
	if detach || periodic || paramjob || multiregion {
		c.Ui.Output("Job start successful")
		if periodic && !paramjob {
			loc, err := job.Periodic.GetLocation()
			if err == nil {
				now := time.Now().In(loc)
				next, err := job.Periodic.Next(now)
				if err != nil {
					c.Ui.Error(fmt.Sprintf("Error determining next launch time: %v", err))
				} else {
					c.Ui.Output(fmt.Sprintf("Approximate next launch time: %s (%s from now)",
						formatTime(next), formatTimeDifference(now, next, time.Second)))
				}
			}
		} else if !paramjob {
			c.Ui.Output("Evaluation ID: " + resp.EvalID)
		}

		return 0
	}

	mon := newMonitor(c.Ui, client, length)
	return mon.monitor(resp.EvalID)
}

func parseFromSubmission(sub *api.JobSubmission) (*api.Job, error) {
	var job *api.Job
	var err error

	switch sub.Format {
	case "hcl2":
		job, err = jobspec2.Parse("", strings.NewReader(sub.Source))
		if err != nil {
			return nil, fmt.Errorf("Unable to parse job submission to re-enable scaling policies: %w", err)
		}

	case "json":
		err = json.Unmarshal([]byte(sub.Source), &job)
		if err != nil {
			return nil, fmt.Errorf("Unable to parse job submission to re-enable scaling policies: %w", err)
		}
	}

	return job, nil
}