Remote support #139

levrado · 2017-01-10T19:17:15Z

Added support for jobType as discussed in #73 with a remote job type

basically it adds the following support:

if user passed job_type: 1 in the create job json, it allows her/him to specify remote_properties:

url, allows to configure to what url to send the request
method, specifying the method to use
body, allows adding a body to the http request
headers, a list of headers as jsons ({"key": "k", "value": "v"}
timeout, after how long to declare the request as failure if there is no response
expected_response_codes, a list of ints to specify which response codes are declared as success

all params have default values and all except for url are optional

levrado · 2017-01-10T19:55:52Z

@ajvb Hi, it seems PR failed for some failure communicating with coveralls and not from my changes, could you please check and rerun?
the message:
Bad response status from coveralls: 422 - {"message":"Couldn't find a repository matching this job.","error":true}

ajvb · 2017-01-10T20:01:06Z

@levrado Thank you for the PR! This is looking great so far.

First off, could we make sure to add comments to some of the new types and functions? As well, can we add some tests that hit a mock http server?
In regards to the coveralls, lets just rip it out in this PR. It has caused me tons of problems, and its easier to just run a coverage test locally.

You will want to delete everything from the test section except these lines: https://github.com/ajvb/kala/blob/master/circle.yml#L26-L28

levrado · 2017-01-10T22:39:20Z

@ajvb added tests, comments and removed coveralls

ajvb · 2017-01-10T22:45:11Z

job/job.go

@@ -94,6 +126,11 @@ type Metadata struct {
 	LastAttemptedRun time.Time `json:"last_attempted_run"`
 }

+type Header struct {


Is there a reason you choose to use a custom type rather than http.Header? https://golang.org/pkg/net/http/#Header

the difference is in how the user request will look like

currently:

{"headers": [{"Content-Type": "text/plain"}]

vs using Google's:

{"headers": {"Content-Type": ["text/plain"]}}

the first seemed a little more natural

I'd much prefer the use of http.Header. No need to re-invent the wheel.

Yes, please use the existing http.Header type

ajvb · 2017-01-10T22:45:50Z

job/job.go

@@ -432,3 +469,18 @@ func (j *Job) ShouldStartWaiting() bool {
 	}
 	return true
 }
+
+func (j *Job) validation() error {
+	if j.JobType == 0 && (j.Name == "" || j.Command == "") {


Can we use the constants defined rather than 0 and 1?

oops, sure :)

elimisteve · 2017-01-10T23:13:44Z

job/job.go

-		log.Errorf(ErrInvalidJob.Error())
-		return ErrInvalidJob
-	}
-


Shouldn't j.validation() be called here instead? Or is it being called elsewhere and I just don't see it?

levrado · 2017-01-22T11:07:51Z

@ajvb can you please update me the status of the review? tnx

ajvb · 2017-01-25T00:30:44Z

hey @levrado, sorry for the delay. I will review today.

ajvb

Two small changes. After those are done I believe it will be ready to merge.

ajvb · 2017-01-25T00:32:37Z

job/job.go

@@ -94,6 +126,11 @@ type Metadata struct {
 	LastAttemptedRun time.Time `json:"last_attempted_run"`
 }

+type Header struct {


I'd much prefer the use of http.Header. No need to re-invent the wheel.

ajvb · 2017-01-25T00:33:00Z

job/job.go

+	LocalJob jobType = 0 + iota
+	RemoteJob
+)
+


Can we add a comment describing RemoteProperties, can be the same as the above one. Mostly for the godocs generated html.

ajvb · 2017-01-25T00:34:59Z

@elimisteve Would love a 👍 from you if you have the time :)

elimisteve

I'm just looking at this quickly, so I'm not seeing the big picture, but other than my comments just given, each line looks good individually :-)

elimisteve · 2017-01-25T00:40:29Z

api/api_test.go

+			"url": "http://example.com",
+		},
+	}
+}


Apply gofmt to this

elimisteve · 2017-01-25T00:41:29Z

job/job.go

-	ErrInvalidJob = errors.New("Invalid Job. Job's must contain a Name and a Command field")
+	ErrInvalidJob = errors.New("Invalid Local Job. Job's must contain a Name and a Command field")
+	ErrInvalidRemoteJob = errors.New("Invalid Remote Job. Job's must contain a Name and a url field")
+	ErrInvalidJobType = errors.New("Invalid Job type. Types supported: 0 for local and 1 for remote")


Again, use gofmt please

elimisteve · 2017-01-25T00:41:57Z

job/job.go

+	JobType jobType `json:"type"`
+
+	// Custom properties for the remote job type
+	RemoteProperties RemoteProperties `json:"remote_proporties"`


Typo: should be remote_propErties, not propOrties

elimisteve · 2017-01-25T00:42:58Z

job/job.go

+	LocalJob jobType = 0 + iota
+	RemoteJob
+)
+


@levrado Good idea, but you should be able to just say iota rather than 0 + iota

elimisteve · 2017-01-25T00:43:59Z

job/job.go

@@ -94,6 +126,11 @@ type Metadata struct {
 	LastAttemptedRun time.Time `json:"last_attempted_run"`
 }

+type Header struct {


Yes, please use the existing http.Header type

elimisteve · 2017-01-25T00:45:57Z

job/job.go

+	} else {
+		return nil
+	}
+}


Reduce code duplication by turning

log.Errorf(ErrInvalidJob.Error()) return ErrInvalidJob

into

err = ErrInvalidJob

then finish with

log.Errorf(err.Error())

(and use that same trick for every else if clause)

elimisteve · 2017-01-26T18:34:46Z

job/runner.go

-		}
+		// Create a new header for our job properties and set the default header
+		j.job.RemoteProperties.Headers = make(http.Header)
+		j.job.RemoteProperties.Headers.Set("Content-Type", jsonContentType)


These 2 lines can be combined into just

j.job.RemoteProperties.Headers = http.Header["Content-Type"] = []string{jsonContentType}

http.Header["Content-Type"] is a list of strings and j.job.RemoteProperties.Headers is a map of string[]string, assignment is not possible

plus it's not very readable friendly to have 2 assignment operators on one line in my opinion

Oops, I meant

j.job.RemoteProperties.Headers = http.Header{"Content-Type": []string{jsonContentType}}

levrado · 2017-01-26T19:17:50Z

@ajvb @elimisteve done changing according to pr comments

circleci is complaining on a data race that i can't seem to replicate on my computer (although i did found a problem in TestOneOffJobs in job_test which i fixed by moving the locking 2 lines up)

could you please have a look and tell me if the error happening with the current master on circleci?

elimisteve · 2017-01-26T19:24:11Z

job/runner.go

+		return errors.New(res.Status)
+	}
+}
+


On line 145, CircleCI says

args, err := shParser.Parse(j.job.Command)

is racing with some other line. Investigating now...

elimisteve · 2017-01-26T19:31:00Z

@levrado Are there any tests that share state? I don't see anything being run concurrently at all, so I'm not sure how and where the race could be coming from.

elimisteve · 2017-01-26T19:34:34Z

job/runner.go

 )

-// Run executes the Job's command, collects metadata around the success
+// Run calls the appropiate run function, collects metadata around the success


*appropRiate

elimisteve · 2017-01-26T19:36:04Z

job/runner.go

+
+// responseTimeout sets a default timeout if none specified
+func (j *JobRunner) responseTimeout() time.Duration {
+	responseTimeout := j.job.RemoteProperties.Timeout


@levrado @ajvb Eventually making this a duration rather than an untyped number that represents seconds would be good

elimisteve · 2017-01-26T19:43:12Z

@levrado Instead of calling every job mock_job, could you number them, or include a high-precision timestamp so that we can tell which mock job is involved in the race?

I don't see much concurrent happening unless the job is unscheduled, in which case it runs immediately:

kala/job/job.go

Line 163 in cd5ec29

go j.Run(cache)

levrado · 2017-01-26T19:48:33Z

@elimisteve the tests that I wrote are with "mock_remote_job" in the job name, those are not my tests that are failing

also it's not happening on my computer (running the same command as circleci) and didn't failed on circleci 2 weeks ago as well.

is it possible to rerun the master branch on circleci? to eliminate that those are my changes causing it

elimisteve · 2017-01-26T19:50:53Z

Found it: shParser is being used concurrently, which isn't safe: a0c7cc9#commitcomment-20631329

levrado · 2017-01-26T20:02:18Z

@elimisteve good catch, weird that it passed ok till now for you guys

changed it to not be reused but be created on every new command run

elimisteve · 2017-01-26T20:06:11Z

@ajvb @levrado I'm not seeing the entire big picture, but in the small at least, LGTM :-) 👍

ajvb

Looks good! Thanks again @levrado

Remote support

levrado added 4 commits January 10, 2017 21:02

added remote job support

51645c3

little cleaning

045c71a

remove reduntant

85579ee

remove more reduntant

8913b82

added tests and removed coveralls from circleci.yml

5c9bcb3

levrado changed the title ~~Remote support (WIP)~~ Remote support Jan 10, 2017

actually validate and fix test

d81076c

ajvb reviewed Jan 10, 2017

View reviewed changes

elimisteve reviewed Jan 10, 2017

View reviewed changes

changed to consts

d097aef

ajvb requested changes Jan 25, 2017

View reviewed changes

elimisteve reviewed Jan 25, 2017

View reviewed changes

changed according to pr comments

9ab60ca

elimisteve reviewed Jan 26, 2017

View reviewed changes

levrado added 2 commits January 26, 2017 21:00

fix to test, avoiding race

c8e911c

no need to init in seperate line

9b985c5

elimisteve reviewed Jan 26, 2017

View reviewed changes

dont reuse shParser

c27d2cd

ajvb approved these changes Feb 12, 2017

View reviewed changes

ajvb merged commit 3e23a94 into ajvb:master Feb 12, 2017

ajvb mentioned this pull request Feb 12, 2017

How to Link Kala to a Rest Url or Golang Function instead of bash command #128

Closed

gwoo pushed a commit to gwoo/kala that referenced this pull request Feb 16, 2017

Merge pull request ajvb#139 from levrado/remote_support

e677b13

Remote support

tooolbox mentioned this pull request Dec 2, 2019

Create a more generic "JobRunner" that will allow for different job types. #73

Closed

Remote support #139

Remote support #139

Conversation

levrado commented Jan 10, 2017 • edited Loading

levrado commented Jan 10, 2017

ajvb commented Jan 10, 2017

levrado commented Jan 10, 2017

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

levrado commented Jan 22, 2017

ajvb commented Jan 25, 2017

ajvb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajvb commented Jan 25, 2017

elimisteve left a 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

levrado Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levrado commented Jan 26, 2017

Choose a reason for hiding this comment

elimisteve commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elimisteve commented Jan 26, 2017

levrado commented Jan 26, 2017 • edited Loading

elimisteve commented Jan 26, 2017

levrado commented Jan 26, 2017

elimisteve commented Jan 26, 2017

ajvb left a comment

Choose a reason for hiding this comment

levrado commented Jan 10, 2017 •

edited

Loading

levrado Jan 26, 2017 •

edited

Loading

levrado commented Jan 26, 2017 •

edited

Loading