-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HCL to JSON api endpoint #4138
HCL to JSON api endpoint #4138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 🎉 🎉 🎉
Go ahead and add a changelog entry like:
IMPROVEMENTS:
...
api: Add /v1/jobs/parse endpoint for rending HCL job files as JSON [GH-2782]
Then run make changelogfmt
to replace the issue number with a link.
Changelog entries are sorted core
first, then alphabetically by category. Within a category they're roughly sorted shortest-to-longest.
api/jobs.go
Outdated
type JobsParseRequest struct { | ||
//JobHCL is an hcl jobspec | ||
JobHCL string | ||
//Canonicalize is a flag as to if the server should return default values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a newline between the previous field and the comment on the next one.
api/jobs.go
Outdated
// Jobs returns a handle on the jobs endpoints. | ||
func (c *Client) Jobs() *Jobs { | ||
return &Jobs{client: c} | ||
} | ||
|
||
// Parse is used to convert the HCL repesentation of a Job to JSON server side | ||
// To parse the HCL client side see package github.com/hashicorp/nomad/jobspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a period between multiple sentences in comments as godoc/godoc.org will run them together into one line.
command/agent/job_endpoint_test.go
Outdated
t.Parallel() | ||
httpTest(t, nil, func(s *TestAgent) { | ||
buf := encodeReq(api.JobsParseRequest{JobHCL: mock.HCL()}) | ||
req, err := http.NewRequest("POST", "/v1/jobs/render", buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/parse
not /render
Although since we're calling the handler directly it doesn't matter.
| Method | Path | Produces | | ||
| ------ | ------------------------- | -------------------------- | | ||
| `POST` | `/v1/jobs/parse` | `application/json` | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the standard blocking/acl section even though neither apply:
| Blocking Queries | ACL Required |
| ---------------- | -------------------------- |
| `No` | `none` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
api/jobs.go
Outdated
// Jobs returns a handle on the jobs endpoints. | ||
func (c *Client) Jobs() *Jobs { | ||
return &Jobs{client: c} | ||
} | ||
|
||
// Parse is used to convert the HCL repesentation of a Job to JSON server side | ||
// To parse the HCL client side see package github.com/hashicorp/nomad/jobspec | ||
func (j *Jobs) Parse(jobHCL string) (*Job, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add canonicalize as a boolean parameter.
Test it in api/jobs_test.go
The parse endpoint accepts a hcl jobspec body within a json object and returns the parsed json object for the job. This allows users to register jobs with the nomad json api without specifically needing a nomad binary to parse their hcl encoded jobspec file.
88bb27a
to
815c9fc
Compare
api/jobs.go
Outdated
@@ -36,11 +36,33 @@ type Jobs struct { | |||
client *Client | |||
} | |||
|
|||
// JobsParseRequest is used for arguments of the /vi/jobs/parse endpoint | |||
type JobsParseRequest struct { | |||
//JobHCL is an hcl jobspec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put a space between forward slashes and the first letter
api/jobs.go
Outdated
// Jobs returns a handle on the jobs endpoints. | ||
func (c *Client) Jobs() *Jobs { | ||
return &Jobs{client: c} | ||
} | ||
|
||
// Parse is used to convert the HCL repesentation of a Job to JSON server side. | ||
// To parse the HCL client side see package github.com/hashicorp/nomad/jobspec | ||
func (j *Jobs) Parse(jobHCL string, canonicalize bool) (*Job, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name it ParseHCL
so we are futureproofed if we want to add a ParseJSON
with canonicalize for people who want to just have defaults set.
api/jobs_test.go
Outdated
|
||
jobs := c.Jobs() | ||
|
||
checkJob := func(job *Job, expected string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected -> expectedRegion
website/source/api/jobs.html.md
Outdated
@@ -231,6 +231,83 @@ $ curl \ | |||
} | |||
``` | |||
|
|||
## Parse Job | |||
|
|||
This endpoint will parse an hcl jobspec and produce the equivalent json encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HCL and JSON capitalized
} | ||
|
||
if args.Canonicalize { | ||
jobStruct.Canonicalize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not capture the warnings returned here and return it: https://github.com/hashicorp/nomad/blob/master/nomad/structs/structs.go#L906-L908
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. Canonicalize()
does not return any errors? https://github.com/hashicorp/nomad/blob/master/api/jobs.go#L585
job.Datacenters[0] != expected.Datacenters[0] { | ||
t.Fatalf("job datacenters is '%s', expected '%s'", | ||
job.Datacenters[0], expected.Datacenters[0]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest checking all fields of the returned job against expected instead of just these three fields.
We have a diff helper in
Line 59 in 5c8b249
func (j *Job) Diff(other *Job, contextual bool) (*JobDiff, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are api.Job
structs. Is there a helper to convert to a structs.Job
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nomad/command/agent/job_endpoint.go
Line 547 in 537b947
func ApiJobToStructJob(job *api.Job) *structs.Job { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cautionary note - not sure if a direct translation like works okay when it comes to unset fields (esp if canonicalize is not called). so it may produce a diff complaining about empty list vs nil. I haven't tried it before, seem worth trying it and using it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some time looking at this and there are a few things:
Diff
errors if IDs don't match (mock.Job() generates a unique ID)- Lots of unset fields yields a somewhat unuseful diff.
- The
mock.HCL()
method doesn't quite mimic themock.Job()
fully.
At some point it feels like I'm testing more of the jobspec.Parse
functionality than the api endpoint. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. It doesn't seem worth it here to use diff for this test given the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer though, I learned more about the structs package 😃
region := job.Region | ||
|
||
if region == nil { | ||
if expectedRegion != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future feel free to use require.NotEmpty
to condense these simple test assertions.
testify is kind of our blessed assertion utility library.
No need to change this PR.
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. |
The parse endpoint accepts a hcl jobspec body within a json object
and returns the parsed json object for the job. This allows users to
register jobs with the nomad json api without specifically needing
a nomad binary to parse their hcl encoded jobspec file.
Fixes #2782