Making the API default missing values #2300

diptanu · 2017-02-09T21:43:42Z

No description provided.

dadgar · 2017-02-09T22:23:22Z

api/jobs.go

+	if p.Spec != nil {
+		p.SpecType = helper.StringToPtr(PeriodicSpecCron)
+	}
+	if p.ProhibitOverlap == nil {


dadgar · 2017-02-09T22:25:13Z

api/jobs.go

+		j.Periodic.Canonicalize()
+	}
+
+	for _, tg := range j.TaskGroups {


Break these up into different functions

dadgar · 2017-02-09T22:32:12Z

api/jobs.go

+				}
+			} else {
+				if task.LogConfig.MaxFiles == nil {
+					task.LogConfig.MaxFiles = helper.IntToPtr(10)


The fields should be handled separately

dadgar · 2017-02-09T22:33:02Z

api/jobs.go

+			if task.Vault != nil {
+				if task.Vault.Env == nil {
+					task.Vault.Env = helper.BoolToPtr(true)
+					task.Vault.ChangeMode = helper.StringToPtr("restart")


These should be handled separately

Handle changemode being signal but signal not being set?

dadgar · 2017-02-09T22:34:56Z

api/jobs_test.go

+	_, err := jobs.Validate(job, nil)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}


Add a failure case

dadgar · 2017-02-09T22:36:17Z

command/agent/job_endpoint.go

+	var out structs.JobValidateResponse
+	if err := s.agent.RPC("Job.Validate", &args, &out); err != nil {
+
+		// Fall back to do local validation


Set that the driver configs were not validated

Default is false, so don't need it.

dadgar · 2017-02-09T22:41:20Z

command/agent/job_endpoint.go

@@ -111,6 +114,40 @@ func (s *HTTPServer) jobPlan(resp http.ResponseWriter, req *http.Request,
 	return out, nil
 }

+func (s *HTTPServer) ValidateJobRequest(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
+	var validateRequest api.JobValidateRequest


Check that the request is PUT/POST

Shouldn't be PUT, we should check for POST

Well looks like client.Write sets the method to PUT.

dadgar · 2017-02-09T22:43:41Z

command/agent/job_endpoint.go

@@ -310,3 +347,86 @@ func (s *HTTPServer) jobDispatchRequest(resp http.ResponseWriter, req *http.Requ
 	setIndex(resp, out.Index)
 	return out, nil
 }
+
+func (s *HTTPServer) apiJobToStructJob(job *api.Job) *structs.Job {
+	if job.Priority == nil {


Why is this not api.Canonicalize() and then just setting each field. It is probably also worth adding apiJobToStructJob which calls apiTaskGroupsToStructTaskGroups and so on.

dadgar · 2017-02-09T22:47:59Z

command/plan.go

@@ -129,12 +112,18 @@ func (c *PlanCommand) Run(args []string) int {
 	}

 	// Force the region to be that of the job.
-	if r := job.Region; r != "" {
+	if r := *job.Region; r != "" {


This can be a nil pointer dereference since the API job has not been canonicalized

dadgar · 2017-02-09T22:56:17Z

nomad/structs/structs.go

+
+// JobValidateResponse is the response from validate request
+type JobValidateResponse struct {
+	DriverConfigValidated bool


Add comments

dadgar · 2017-02-09T22:56:42Z

api/jobs.go

@@ -371,6 +487,11 @@ func (j *Job) AddPeriodicConfig(cfg *PeriodicConfig) *Job {
 	return j
 }

+// JobValidateRequest is used to validate a job
+type JobValidateRequest struct {
+	Job *Job


WriteRequest?

dadgar · 2017-02-09T22:59:32Z

jobspec/parse.go

 				Name:          t.Name,
 				Count:         1,
-				EphemeralDisk: structs.DefaultEphemeralDisk(),
-				Tasks:         []*structs.Task{t},
+				EphemeralDisk: &api.EphemeralDisk{SizeMB: 300},


Those should be pointers ^ (we decided on all of them being pointers)

dadgar · 2017-02-09T23:00:46Z

jobspec/parse.go

@@ -317,7 +324,7 @@ func parseGroups(result *structs.Job, list *ast.ObjectList) error {
 		}

 		// Parse ephemeral disk
-		g.EphemeralDisk = structs.DefaultEphemeralDisk()
+		g.EphemeralDisk = &api.EphemeralDisk{SizeMB: 300}


dadgar · 2017-02-09T23:02:19Z

jobspec/parse.go

 	}

 	// Combine the parsed resources with a default resource block.
-	min := structs.DefaultResources()
+	min := &api.Resources{


Create a min resources for API

dadgar · 2017-02-14T18:17:04Z

api/jobs.go

+	if j.JobModifyIndex == nil {
+		j.JobModifyIndex = helper.Uint64ToPtr(0)
+	}
+	if j.Periodic != nil {


Similar block for update strategy and parameterized?

dadgar · 2017-02-14T18:26:27Z

api/tasks.go

+	}
+	if tmpl.Perms == nil {
+		tmpl.Perms = helper.StringToPtr("0644")
+	}


Can you add a case for if *tmpl.ChangeMode == "signal" && tmpl.ChangeSignal == nil { tmpl.ChangeSignal = helper.StringToPtr("SIGHUP") }

If it is set upper case the signal

dadgar · 2017-02-14T18:28:33Z

api/tasks.go

+		v.ChangeMode = helper.StringToPtr("restart")
+	}
+	if v.ChangeSignal == nil {
+		v.ChangeSignal = helper.StringToPtr("sighup")


Capitalize the signal both in that helper and put an else if it is set and upper case it

dadgar · 2017-02-14T18:31:42Z

api/resources.go

+		r.Networks = other.Networks
+	}
+}
+
 type Port struct {
 	Label string


Make these pointers and give a default value to the mbits (10 seems fine)

dadgar · 2017-02-14T18:33:11Z

command/agent/job_endpoint.go

+
+	j.Constraints = make([]*structs.Constraint, len(job.Constraints))
+	for i, c := range job.Constraints {
+		j.Constraints[i] = &structs.Constraint{


Should be a method so it can be reused

dadgar · 2017-02-14T18:35:48Z

api/tasks.go

+	if g.RestartPolicy == nil {
+		switch jobType {
+		case "service", "system":
+			g.RestartPolicy = &RestartPolicy{


Make these pointers and add a merge method. Get the default and the user submitted one and merge them

dadgar · 2017-02-14T18:41:02Z

command/validate.go


 	// Check that the job is valid
-	if err := job.Validate(); err != nil {
+	if _, _, err := client.Jobs().Validate(job, nil); err != nil {


Put a warning message in orange if the drivers were not validated

dadgar · 2017-02-14T18:42:13Z

nomad/job_endpoint.go

@@ -285,6 +285,25 @@ func (j *Job) Summary(args *structs.JobSummaryRequest,
 	return j.srv.blockingRPC(&opts)
 }

+// Validate validates a job
+func (j *Job) Validate(args *structs.JobValidateRequest,


HTTP documentation

schmichael · 2017-02-15T00:14:33Z

command/agent/job_endpoint.go

+	s.parseRegion(req, &args.Region)
+
+	var out structs.JobValidateResponse
+	if err := s.agent.RPC("Job.Validate", &args, &out); err != nil {


~~Should we do something with this err even if no other errs happen?~~

Update: no need!

schmichael · 2017-02-15T00:14:48Z

command/agent/job_endpoint.go

+				}
+			}
+		} else {
+			out.ValidationErrors = append(out.ValidationErrors, vErr.Error())


Isn't vErr nil here?

schmichael · 2017-02-15T00:15:12Z

command/agent/job_endpoint.go

+		// Fall back to do local validation
+		args.Job.Canonicalize()
+		if vErr := args.Job.Validate(); vErr != nil {
+			if merr, ok := err.(*multierror.Error); ok {


Did you mean vErr here?

schmichael · 2017-02-15T00:44:48Z

jobspec/parse.go

+			MaxFiles:      helper.IntToPtr(10),
+			MaxFileSizeMB: helper.IntToPtr(10),
+		}
+


Why not use api.DefaultLogConfig()?

…actor-parser

phinze · 2017-03-03T04:38:29Z

Hi folks!

It seems this PR changes nomad validate to require that an agent be running? Our CI runs nomad validate on our job files - do we need to update it to spin up a local agent for itself after this change?

If all this is accurate, I think we should call this out in the "Backwards Incompatibilities" section of the changelog.

dadgar · 2017-03-03T23:48:09Z

@phinze Hey this was not suppose to happen! It was an oversight and 0.5.5 RC2 will restore the original behavior.

github-actions · 2023-04-04T02:12:02Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

diptanu force-pushed the refactor-parser branch from 67dd49f to 47f7e2e Compare February 9, 2017 21:45

dadgar reviewed Feb 9, 2017

View reviewed changes

diptanu force-pushed the refactor-parser branch 15 times, most recently from 7ee7cad to 57b5fdf Compare February 14, 2017 02:39

dadgar reviewed Feb 14, 2017

View reviewed changes

diptanu force-pushed the refactor-parser branch 4 times, most recently from 08ae0a4 to e66827b Compare February 14, 2017 23:19

schmichael requested changes Feb 15, 2017

View reviewed changes

diptanu force-pushed the refactor-parser branch from e66827b to 5575e54 Compare February 15, 2017 18:26

schmichael approved these changes Feb 15, 2017

View reviewed changes

diptanu force-pushed the refactor-parser branch 4 times, most recently from bd7dbc7 to 14052ed Compare February 15, 2017 21:23

diptanu force-pushed the refactor-parser branch 2 times, most recently from fd28fad to a2d7651 Compare February 15, 2017 21:35

diptanu and others added 10 commits February 16, 2017 13:52

Making the job spec return api.Job

f551dcb

Added tests

5502a66

Added docs

79d5181

Fixed the nomad status command

b56764a

Removing un-neccessary conversions

67e6098

Use RC to see if cause of failure

7eac718

Making the job register endpoint use api.Job

592bc73

Fix detection of missing port

ea79a14

Flatmap

b6e0d12

Refactor test and fix them

6debb3e

dadgar force-pushed the refactor-parser branch from 91dc597 to 6debb3e Compare February 16, 2017 22:00

dadgar and others added 8 commits February 16, 2017 14:29

Fix CLI handling of parameterized/periodic jobs

ef00766

Fixed cli tests

8557a89

Merge branch 'refactor-parser' of github.com:hashicorp/nomad into ref…

aa820e9

…actor-parser

Downgrading to Go 1.7

00f8237

Merge branch 'master' into refactor-parser

76ab4b6

Bump

0ba19ff

Add defaulting test and fix types on the agent job endpoint tests

503746c

fix pointer

7cef0d7

dadgar merged commit 139bb3d into master Feb 21, 2017

dadgar deleted the refactor-parser branch February 21, 2017 00:22

github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making the API default missing values #2300

Making the API default missing values #2300

diptanu commented Feb 9, 2017 •

edited

Loading

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

diptanu Feb 9, 2017

dadgar Feb 9, 2017

diptanu Feb 9, 2017

diptanu Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 9, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

dadgar Feb 14, 2017

schmichael Feb 15, 2017 •

edited

Loading

schmichael Feb 15, 2017

schmichael Feb 15, 2017

schmichael Feb 15, 2017

phinze commented Mar 3, 2017

dadgar commented Mar 3, 2017

github-actions bot commented Apr 4, 2023

Making the API default missing values #2300

Making the API default missing values #2300

Conversation

diptanu commented Feb 9, 2017 • edited Loading

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

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

schmichael Feb 15, 2017 • edited Loading

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

phinze commented Mar 3, 2017

dadgar commented Mar 3, 2017

github-actions bot commented Apr 4, 2023

diptanu commented Feb 9, 2017 •

edited

Loading

schmichael Feb 15, 2017 •

edited

Loading